diff options
author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2019-10-30 13:56:41 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2019-10-30 13:56:41 +0000 |
commit | f2f06b5048e31dfe5cbb5489ab1d4da585b1f60b (patch) | |
tree | a9c16c3a98ea05446f9e5b8d017a993b66e07ede | |
parent | 50d93f8d1686950fc58dda4823c4835fd0d8c14b (diff) | |
parent | 82ff748612cb7b5c3eddd9ede8a2546146ef07ab (diff) | |
download | gitlab-ce-f2f06b5048e31dfe5cbb5489ab1d4da585b1f60b.tar.gz |
Merge remote-tracking branch 'dev/12-3-stable' into 12-3-stable
84 files changed, 1405 insertions, 174 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ec3b4e56a1..bc35154362c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.3.6 + +### Security (14 changes) + +- Standardize error response when route is missing. +- Do not display project labels that are not visible for user accessing group labels. +- Show cross-referenced label and milestones in issues' activities only to authorized users. +- Analyze incoming GraphQL queries and check for recursion. +- Disallow unprivileged users from commenting on private repository commits. +- Don't allow maintainers of a target project to delete the source branch of a merge request from a fork. +- Require Maintainer permission on group where project is transferred to. +- Don't leak private members in project member autocomplete suggestions. +- Return 404 on LFS request if project doesn't exist. +- Mask sentry auth token in Error Tracking dashboard. +- Fixes a Open Redirect issue in `InternalRedirect`. +- Sanitize search text to prevent XSS. +- Sanitize all wiki markup formats with GitLab sanitization pipelines. +- Fix stored XSS issue for grafana_url. + + ## 12.3.5 - No changes. @@ -1 +1 @@ -12.3.5 +12.3.6 diff --git a/app/assets/javascripts/project_find_file.js b/app/assets/javascripts/project_find_file.js index e73a828c0ae..5f9a4c1bf07 100644 --- a/app/assets/javascripts/project_find_file.js +++ b/app/assets/javascripts/project_find_file.js @@ -5,6 +5,7 @@ import fuzzaldrinPlus from 'fuzzaldrin-plus'; import axios from '~/lib/utils/axios_utils'; import flash from '~/flash'; import { __ } from '~/locale'; +import sanitize from 'sanitize-html'; // highlight text(awefwbwgtc -> <b>a</b>wefw<b>b</b>wgt<b>c</b> ) const highlighter = function(element, text, matches) { @@ -74,7 +75,7 @@ export default class ProjectFindFile { findFile() { var result, searchText; - searchText = this.inputElement.val(); + searchText = sanitize(this.inputElement.val()); result = searchText.length > 0 ? fuzzaldrinPlus.filter(this.filePaths, searchText) : this.filePaths; return this.renderList(result, searchText); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 9a7859fc687..501c32e99da 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,7 +14,7 @@ class ApplicationController < ActionController::Base include SessionlessAuthentication include ConfirmEmailWarning - before_action :authenticate_user! + before_action :authenticate_user!, except: [:route_not_found] before_action :enforce_terms!, if: :should_enforce_terms? before_action :validate_user_service_ticket! before_action :check_password_expiration @@ -92,7 +92,9 @@ class ApplicationController < ActionController::Base if current_user not_found else - authenticate_user! + store_location_for(:user, request.fullpath) unless request.xhr? + + redirect_to new_user_session_path, alert: I18n.t('devise.failure.unauthenticated') end end diff --git a/app/controllers/concerns/internal_redirect.rb b/app/controllers/concerns/internal_redirect.rb index 99bbfd56516..a35bc19aa37 100644 --- a/app/controllers/concerns/internal_redirect.rb +++ b/app/controllers/concerns/internal_redirect.rb @@ -6,7 +6,7 @@ module InternalRedirect def safe_redirect_path(path) return unless path # Verify that the string starts with a `/` and a known route character. - return unless path =~ %r{^/[-\w].*$} + return unless path =~ %r{\A/[-\w].*\z} uri = URI(path) # Ignore anything path of the redirect except for the path, querystring and, diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index 733265f4099..417bb169f39 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -34,6 +34,7 @@ module LfsRequest end def lfs_check_access! + return render_lfs_not_found unless project return if download_request? && lfs_download_access? return if upload_request? && lfs_upload_access? diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb index e523942ea4c..027cdc4fc78 100644 --- a/app/finders/labels_finder.rb +++ b/app/finders/labels_finder.rb @@ -51,7 +51,7 @@ class LabelsFinder < UnionFinder end label_ids << Label.where(group_id: projects.group_ids) - label_ids << Label.where(project_id: projects.select(:id)) unless only_group_labels? + label_ids << Label.where(project_id: ids_user_can_read_labels(projects)) unless only_group_labels? end label_ids @@ -188,4 +188,10 @@ class LabelsFinder < UnionFinder groups.select { |group| authorized_to_read_labels?(group) } end end + + # rubocop: disable CodeReuse/ActiveRecord + def ids_user_can_read_labels(projects) + Project.where(id: projects.select(:id)).ids_with_issuables_available_for(current_user) + end + # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index 4c8612c8f2e..1899278ff3c 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -18,15 +18,15 @@ class GitlabSchema < GraphQL::Schema use Gitlab::Graphql::GenericTracing query_analyzer Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer.new - - query(Types::QueryType) - - default_max_page_size 100 + query_analyzer Gitlab::Graphql::QueryAnalyzers::RecursionAnalyzer.new max_complexity DEFAULT_MAX_COMPLEXITY max_depth DEFAULT_MAX_DEPTH - mutation(Types::MutationType) + query Types::QueryType + mutation Types::MutationType + + default_max_page_size 100 class << self def multiplex(queries, **kwargs) diff --git a/app/helpers/markup_helper.rb b/app/helpers/markup_helper.rb index d76a0f3a3b8..e2524938e10 100644 --- a/app/helpers/markup_helper.rb +++ b/app/helpers/markup_helper.rb @@ -133,15 +133,7 @@ module MarkupHelper issuable_state_filter_enabled: true ) - html = - case wiki_page.format - when :markdown - markdown_unsafe(text, context) - when :asciidoc - asciidoc_unsafe(text) - else - wiki_page.formatted_content.html_safe - end + html = markup_unsafe(wiki_page.path, text, context) prepare_for_rendering(html, context) end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 92526def144..a14445511a7 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -6,6 +6,13 @@ class ApplicationSetting < ApplicationRecord include TokenAuthenticatable include ChronicDurationAttribute + GRAFANA_URL_RULES = { + allow_localhost: true, + allow_local_network: true, + enforce_sanitization: true, + require_absolute: false + }.freeze + add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required } add_authentication_token_field :health_check_access_token add_authentication_token_field :static_objects_external_storage_auth_token @@ -48,6 +55,11 @@ class ApplicationSetting < ApplicationRecord allow_nil: false, qualified_domain_array: true + validates :grafana_url, + allow_blank: true, + allow_nil: true, + addressable_url: GRAFANA_URL_RULES + validates :session_expire_delay, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0 } @@ -65,7 +77,6 @@ class ApplicationSetting < ApplicationRecord validates :after_sign_out_path, allow_blank: true, addressable_url: true - validates :admin_notification_email, devise_email: true, allow_blank: true @@ -303,6 +314,14 @@ class ApplicationSetting < ApplicationRecord current_without_cache end + def grafana_url + if Gitlab::UrlBlocker.blocked_url?(self[:grafana_url], GRAFANA_URL_RULES) + ApplicationSetting.column_defaults["grafana_url"] + else + self[:grafana_url] + end + end + # By default, the backend is Rails.cache, which uses # ActiveSupport::Cache::RedisStore. Since loading ApplicationSetting # can cause a significant amount of load on Redis, let's cache it in diff --git a/app/models/concerns/mentionable/reference_regexes.rb b/app/models/concerns/mentionable/reference_regexes.rb index fec31cd262b..f44a674b3c9 100644 --- a/app/models/concerns/mentionable/reference_regexes.rb +++ b/app/models/concerns/mentionable/reference_regexes.rb @@ -13,7 +13,9 @@ module Mentionable def self.other_patterns [ Commit.reference_pattern, - MergeRequest.reference_pattern + MergeRequest.reference_pattern, + Label.reference_pattern, + Milestone.reference_pattern ] end diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 0d066d0d99f..b8525f7b135 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -16,6 +16,7 @@ class Discussion :commit_id, :for_commit?, :for_merge_request?, + :noteable_ability_name, :to_ability_name, :editable?, :visible_for?, diff --git a/app/models/member.rb b/app/models/member.rb index e2d26773d45..2654453cf3f 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -8,6 +8,7 @@ class Member < ApplicationRecord include Gitlab::Access include Presentable include Gitlab::Utils::StrongMemoize + include FromUnion attr_accessor :raw_invite_token diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 63133ca285b..7dc0f772ee4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -69,6 +69,14 @@ class MergeRequest < ApplicationRecord has_many :merge_request_assignees has_many :assignees, class_name: "User", through: :merge_request_assignees + KNOWN_MERGE_PARAMS = [ + :auto_merge_strategy, + :should_remove_source_branch, + :force_remove_source_branch, + :commit_message, + :squash_commit_message, + :sha + ].freeze serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize after_create :ensure_merge_request_diff diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 916c11a8d03..75c004b98f2 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -262,6 +262,10 @@ class Milestone < ApplicationRecord end alias_method :resource_parent, :parent + def to_ability_name + model_name.singular + end + def group_milestone? group_id.present? end diff --git a/app/models/note.rb b/app/models/note.rb index b1829e71017..a0c5414aede 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -350,6 +350,10 @@ class Note < ApplicationRecord end def to_ability_name + model_name.singular + end + + def noteable_ability_name for_snippet? ? noteable.class.name.underscore : noteable_type.demodulize.underscore end diff --git a/app/models/project.rb b/app/models/project.rb index 5c3bf4a3b5d..e4341bcee6c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -591,11 +591,11 @@ class Project < ApplicationRecord joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id) end - # Returns ids of projects with milestones available for given user + # Returns ids of projects with issuables available for given user # - # Used on queries to find milestones which user can see - # For example: Milestone.where(project_id: ids_with_milestone_available_for(user)) - def ids_with_milestone_available_for(user) + # Used on queries to find milestones or labels which user can see + # For example: Milestone.where(project_id: ids_with_issuables_available_for(user)) + def ids_with_issuables_available_for(user) with_issues_enabled = with_issues_available_for_user(user).select(:id) with_merge_requests_enabled = with_merge_requests_available_for_user(user).select(:id) @@ -1242,6 +1242,10 @@ class Project < ApplicationRecord end end + def to_ability_name + model_name.singular + end + # rubocop: disable CodeReuse/ServiceClass def execute_hooks(data, hooks_scope = :push_hooks) run_after_commit_or_now do diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 8ec90ca25d3..e47c9081ad3 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -10,6 +10,7 @@ class SystemNoteMetadata < ApplicationRecord commit cross_reference close duplicate moved merge + label milestone ].freeze ICON_TYPES = %w[ diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index cd4c7895587..1b6d8fc47a7 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -138,6 +138,12 @@ class WikiPage @version ||= @page.version end + def path + return unless persisted? + + @path ||= @page.path + end + def versions(options = {}) return [] unless persisted? diff --git a/app/policies/commit_policy.rb b/app/policies/commit_policy.rb index 4d4f0ba9267..4b358c45ec2 100644 --- a/app/policies/commit_policy.rb +++ b/app/policies/commit_policy.rb @@ -4,4 +4,5 @@ class CommitPolicy < BasePolicy delegate { @subject.project } rule { can?(:download_code) }.enable :read_commit + rule { ~can?(:read_commit) }.prevent :create_note end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index f56ac0a5279..2ad19aa7c3a 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -128,6 +128,8 @@ class GroupPolicy < BasePolicy rule { owner | admin }.enable :read_statistics + rule { maintainer & can?(:create_projects) }.enable :transfer_projects + def access_level return GroupMember::NO_ACCESS if @user.nil? diff --git a/app/policies/namespace_policy.rb b/app/policies/namespace_policy.rb index fd3bdddded6..350dd208499 100644 --- a/app/policies/namespace_policy.rb +++ b/app/policies/namespace_policy.rb @@ -15,6 +15,8 @@ class NamespacePolicy < BasePolicy end rule { personal_project & ~can_create_personal_project }.prevent :create_projects + + rule { (owner | admin) & can?(:create_projects) }.enable :transfer_projects end NamespacePolicy.prepend_if_ee('EE::NamespacePolicy') diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index b2af6c874c7..dcde8cefa0d 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -9,7 +9,7 @@ class NotePolicy < BasePolicy condition(:editable, scope: :subject) { @subject.editable? } - condition(:can_read_noteable) { can?(:"read_#{@subject.to_ability_name}") } + condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") } condition(:is_visible) { @subject.visible_for?(@user) } diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb index e06659a39cd..e08b4ac2260 100644 --- a/app/services/auto_merge/base_service.rb +++ b/app/services/auto_merge/base_service.rb @@ -3,12 +3,13 @@ module AutoMerge class BaseService < ::BaseService include Gitlab::Utils::StrongMemoize + include MergeRequests::AssignsMergeParams def execute(merge_request) - merge_request.merge_params.merge!(params) + assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy)) + merge_request.auto_merge_enabled = true merge_request.merge_user = current_user - merge_request.auto_merge_strategy = strategy return :failed unless merge_request.save @@ -21,7 +22,7 @@ module AutoMerge end def update(merge_request) - merge_request.merge_params.merge!(params) + assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy)) return :failed unless merge_request.save diff --git a/app/services/concerns/merge_requests/assigns_merge_params.rb b/app/services/concerns/merge_requests/assigns_merge_params.rb new file mode 100644 index 00000000000..bd870d9a1e7 --- /dev/null +++ b/app/services/concerns/merge_requests/assigns_merge_params.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module MergeRequests + module AssignsMergeParams + def self.included(klass) + raise "#{self} can not be included in #{klass} without implementing #current_user" unless klass.method_defined?(:current_user) + end + + def assign_allowed_merge_params(merge_request, merge_params) + known_merge_params = merge_params.to_h.with_indifferent_access.slice(*MergeRequest::KNOWN_MERGE_PARAMS) + + # Not checking `MergeRequest#can_remove_source_branch` as that includes + # other checks that aren't needed here. + known_merge_params.delete(:force_remove_source_branch) unless current_user.can?(:push_code, merge_request.source_project) + + merge_request.merge_params.merge!(known_merge_params) + + # Delete the known params now that they're assigned, so we don't try to + # assign them through an `#assign_attributes` later. + # They could be coming in as strings or symbols + merge_params.to_h.with_indifferent_access.except!(*MergeRequest::KNOWN_MERGE_PARAMS) + end + end +end diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb index 8d08f0cda94..92d4ef85ecf 100644 --- a/app/services/error_tracking/list_projects_service.rb +++ b/app/services/error_tracking/list_projects_service.rb @@ -32,7 +32,7 @@ module ErrorTracking project_slug: 'proj' ) - setting.token = params[:token] + setting.token = token(setting) setting.enabled = true end end @@ -40,5 +40,12 @@ module ErrorTracking def can_read? can?(current_user, :read_sentry_issue, project) end + + def token(setting) + # Use param token if not masked, otherwise use database token + return params[:token] unless /\A\*+\z/.match?(params[:token]) + + setting.token + end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 7d4227e4a41..aacc3d6831e 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -2,6 +2,8 @@ module MergeRequests class BaseService < ::IssuableBaseService + include MergeRequests::AssignsMergeParams + def create_note(merge_request, state = merge_request.state) SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, state, nil) end @@ -29,6 +31,18 @@ module MergeRequests private + def create(merge_request) + self.params = assign_allowed_merge_params(merge_request, params) + + super + end + + def update(merge_request) + self.params = assign_allowed_merge_params(merge_request, params) + + super + end + def handle_wip_event(merge_request) if wip_event = params.delete(:wip_event) # We update the title that is provided in the params or we use the mr title diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 214f145d09b..bf4da01723b 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -10,13 +10,14 @@ module MergeRequests # TODO: this should handle all quick actions that don't have side effects # https://gitlab.com/gitlab-org/gitlab-foss/issues/53658 merge_quick_actions_into_params!(merge_request, only: [:target_branch]) - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) if params.has_key?(:force_remove_source_branch) # Assign the projects first so we can use policies for `filter_params` merge_request.author = current_user merge_request.source_project = find_source_project merge_request.target_project = find_target_project + self.params = assign_allowed_merge_params(merge_request, params) + filter_params(merge_request) # merge_request.assign_attributes(...) below is a Rails diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 1c730232abb..9a37a0330fc 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -9,7 +9,6 @@ module MergeRequests merge_request.target_project = @project merge_request.source_project = @source_project merge_request.source_branch = params[:source_branch] - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) create(merge_request) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 4acc3f1981a..3bdab0b0931 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,10 +16,6 @@ module MergeRequests params.delete(:force_remove_source_branch) end - if params.has_key?(:force_remove_source_branch) - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) - end - handle_wip_event(merge_request) update_task_event(merge_request) || update(merge_request) end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index ed357aa0392..2f8c5ffddd9 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -281,7 +281,7 @@ class NotificationService end def send_new_note_notifications(note) - notify_method = "note_#{note.to_ability_name}_email".to_sym + notify_method = "note_#{note.noteable_ability_name}_email".to_sym recipients = NotificationRecipientService.build_new_note_recipients(note) recipients.each do |recipient| diff --git a/app/services/projects/operations/update_service.rb b/app/services/projects/operations/update_service.rb index dd72c2844c2..44c431f0ec1 100644 --- a/app/services/projects/operations/update_service.rb +++ b/app/services/projects/operations/update_service.rb @@ -34,15 +34,17 @@ module Projects organization_slug: settings.dig(:project, :organization_slug) ) - { + params = { error_tracking_setting_attributes: { api_url: api_url, - token: settings[:token], enabled: settings[:enabled], project_name: settings.dig(:project, :name), organization_name: settings.dig(:project, :organization_name) } } + params[:error_tracking_setting_attributes][:token] = settings[:token] unless /\A\*+\z/.match?(settings[:token]) # Don't update token if we receive masked value + + params end end end diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index 7080f388e53..1cd81fe37c7 100644 --- a/app/services/projects/participants_service.rb +++ b/app/services/projects/participants_service.rb @@ -7,16 +7,69 @@ module Projects def execute(noteable) @noteable = noteable - participants = noteable_owner + participants_in_noteable + all_members + groups + project_members + participants = + noteable_owner + + participants_in_noteable + + all_members + + groups + + project_members + participants.uniq end def project_members - @project_members ||= sorted(project.team.members) + @project_members ||= sorted(get_project_members) + end + + def get_project_members + members = Member.from_union([project_members_through_ancestral_groups, + project_members_through_invited_groups, + individual_project_members]) + + User.id_in(members.select(:user_id)) end def all_members [{ username: "all", name: "All Project and Group Members", count: project_members.count }] end + + private + + def project_members_through_invited_groups + groups_with_ancestors_ids = Gitlab::ObjectHierarchy + .new(visible_groups) + .base_and_ancestors + .pluck_primary_key + + GroupMember + .active_without_invites_and_requests + .with_source_id(groups_with_ancestors_ids) + end + + def visible_groups + visible_groups = project.invited_groups + + unless project_owner? + visible_groups = visible_groups.public_or_visible_to_user(current_user) + end + + visible_groups + end + + def project_members_through_ancestral_groups + project.group.present? ? project.group.members_with_parents : Member.none + end + + def individual_project_members + project.project_members + end + + def project_owner? + if project.group.present? + project.group.owners.include?(current_user) + else + project.namespace.owner == current_user + end + end end end diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index 4b3aca58dd7..525fc18b312 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -98,7 +98,7 @@ module Projects @new_namespace && can?(current_user, :change_namespace, project) && @new_namespace.id != project.namespace_id && - current_user.can?(:create_projects, @new_namespace) + current_user.can?(:transfer_projects, @new_namespace) end def update_namespace_and_visibility(to_namespace) diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb index 300bd01ed22..179abde17ff 100644 --- a/app/validators/addressable_url_validator.rb +++ b/app/validators/addressable_url_validator.rb @@ -55,7 +55,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator ascii_only: false, enforce_user: false, enforce_sanitization: false, - dns_rebind_protection: false + dns_rebind_protection: false, + require_absolute: true }.freeze DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({ diff --git a/app/views/projects/settings/operations/_error_tracking.html.haml b/app/views/projects/settings/operations/_error_tracking.html.haml index 583fc08f375..589d3037eba 100644 --- a/app/views/projects/settings/operations/_error_tracking.html.haml +++ b/app/views/projects/settings/operations/_error_tracking.html.haml @@ -17,4 +17,4 @@ project: error_tracking_setting_project_json, api_host: setting.api_host, enabled: setting.enabled.to_json, - token: setting.token } } + token: setting.token.present? ? '*' * 12 : nil } } diff --git a/doc/user/project/members/share_project_with_groups.md b/doc/user/project/members/share_project_with_groups.md index 9340d239677..79fb2ea50a0 100644 --- a/doc/user/project/members/share_project_with_groups.md +++ b/doc/user/project/members/share_project_with_groups.md @@ -44,6 +44,10 @@ Admins are able to share projects with any group in the system. In the example above, the maximum access level of 'Developer' for members from 'Engineering' means that users with higher access levels in 'Engineering' ('Maintainer' or 'Owner') will only have 'Developer' access to 'Project Acme'. +## Sharing public project with private group + +When sharing a public project with a private group, owners and maintainers of the project will see the name of the group in the `members` page. Owners will also have the possibility to see members of the private group they don't have access to when mentioning them in the issue or merge request. + ## Share project with group lock It is possible to prevent projects in a group from [sharing diff --git a/lib/gitlab/graphql/query_analyzers/recursion_analyzer.rb b/lib/gitlab/graphql/query_analyzers/recursion_analyzer.rb new file mode 100644 index 00000000000..ccf9e597307 --- /dev/null +++ b/lib/gitlab/graphql/query_analyzers/recursion_analyzer.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +# Recursive queries, with relatively low effort, can quickly spiral out of control exponentially +# and may not be picked up by depth and complexity alone. +module Gitlab + module Graphql + module QueryAnalyzers + class RecursionAnalyzer + IGNORED_FIELDS = %w(node edges ofType).freeze + RECURSION_THRESHOLD = 2 + + def initial_value(query) + { + recurring_fields: {} + } + end + + def call(memo, visit_type, irep_node) + return memo if skip_node?(irep_node) + + node_name = irep_node.ast_node.name + times_encountered = memo[node_name] || 0 + + if visit_type == :enter + times_encountered += 1 + memo[:recurring_fields][node_name] = times_encountered if recursion_too_deep?(node_name, times_encountered) + else + times_encountered -= 1 + end + + memo[node_name] = times_encountered + memo + end + + def final_value(memo) + recurring_fields = memo[:recurring_fields] + recurring_fields = recurring_fields.select { |k, v| recursion_too_deep?(k, v) } + if recurring_fields.any? + GraphQL::AnalysisError.new("Recursive query - too many of fields '#{recurring_fields}' detected in single branch of the query") + end + end + + private + + def recursion_too_deep?(node_name, times_encountered) + return if IGNORED_FIELDS.include?(node_name) + + times_encountered > recursion_threshold + end + + def skip_node?(irep_node) + ast_node = irep_node.ast_node + !ast_node.is_a?(GraphQL::Language::Nodes::Field) || ast_node.selections.empty? + end + + def recursion_threshold + RECURSION_THRESHOLD + end + end + end + end +end diff --git a/lib/gitlab/other_markup.rb b/lib/gitlab/other_markup.rb index bc467486eee..0dd6b8a809c 100644 --- a/lib/gitlab/other_markup.rb +++ b/lib/gitlab/other_markup.rb @@ -10,7 +10,7 @@ module Gitlab def self.render(file_name, input, context) html = GitHub::Markup.render(file_name, input) .force_encoding(input.encoding) - context[:pipeline] = :markup + context[:pipeline] ||= :markup html = Banzai.render(html, context) diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 93e172299b9..bca9b385211 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -163,7 +163,7 @@ module Gitlab return Milestone.none if project_ids.nil? authorized_project_ids_relation = - Project.where(id: project_ids).ids_with_milestone_available_for(current_user) + Project.where(id: project_ids).ids_with_issuables_available_for(current_user) milestones.where(project_id: authorized_project_ids_relation) end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 4285b2675c5..94cef820f0d 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -11,11 +11,14 @@ module Gitlab # Validates the given url according to the constraints specified by arguments. # # ports - Raises error if the given URL port does is not between given ports. - # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is true. - # allow_local_network - Raises error if URL resolves to a link-local address and argument is true. + # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is false. + # allow_local_network - Raises error if URL resolves to a link-local address and argument is false. # ascii_only - Raises error if URL has unicode characters and argument is true. # enforce_user - Raises error if URL user doesn't start with alphanumeric characters and argument is true. # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true. + # require_absolute - Raises error if URL is not absolute and argument is true. + # Allow relative URLs beginning with slash when argument is false + # Raises error if relative URL does not begin with slash and argument is false # # Returns an array with [<uri>, <original-hostname>]. # rubocop:disable Metrics/ParameterLists @@ -28,7 +31,8 @@ module Gitlab ascii_only: false, enforce_user: false, enforce_sanitization: false, - dns_rebind_protection: true) + dns_rebind_protection: true, + require_absolute: true) # rubocop:enable Metrics/ParameterLists return [nil, nil] if url.nil? @@ -42,10 +46,11 @@ module Gitlab ports: ports, enforce_sanitization: enforce_sanitization, enforce_user: enforce_user, - ascii_only: ascii_only + ascii_only: ascii_only, + require_absolute: require_absolute ) - address_info = get_address_info(uri, dns_rebind_protection) + address_info = get_address_info(uri, dns_rebind_protection) if require_absolute || uri.absolute? return [uri, nil] unless address_info ip_address = ip_address(address_info) @@ -95,12 +100,14 @@ module Gitlab address_info.first&.ip_address end - def validate_uri(uri:, schemes:, ports:, enforce_sanitization:, enforce_user:, ascii_only:) + def validate_uri(uri:, schemes:, ports:, enforce_sanitization:, enforce_user:, ascii_only:, require_absolute:) validate_html_tags(uri) if enforce_sanitization + validate_absolute(uri) if require_absolute + validate_relative(uri) unless require_absolute return if internal?(uri) - validate_scheme(uri.scheme, schemes) + validate_scheme(uri.scheme, schemes, require_absolute) validate_port(get_port(uri), ports) if ports.any? validate_user(uri.user) if enforce_user validate_hostname(uri.hostname) @@ -177,8 +184,20 @@ module Gitlab raise BlockedUrlError, "Only allowed ports are #{ports.join(', ')}, and any over 1024" end - def validate_scheme(scheme, schemes) - if scheme.blank? || (schemes.any? && !schemes.include?(scheme)) + def validate_absolute(uri) + return if uri.absolute? + + raise BlockedUrlError, 'must be absolute' + end + + def validate_relative(uri) + return if uri.absolute? || uri.path.starts_with?('/') + + raise BlockedUrlError, 'relative path must begin with a / (slash)' + end + + def validate_scheme(scheme, schemes, require_absolute) + if (require_absolute && scheme.blank?) || (schemes.any? && !schemes.include?(scheme)) raise BlockedUrlError, "Only allowed schemes are #{schemes.join(', ')}" end end @@ -237,9 +256,10 @@ module Gitlab end def internal_web?(uri) - uri.scheme == config.gitlab.protocol && - uri.hostname == config.gitlab.host && - (uri.port.blank? || uri.port == config.gitlab.port) + (uri.scheme.blank? && uri.hostname.blank? && uri.port.blank? && uri.path.starts_with?('/')) || + (uri.scheme == config.gitlab.protocol && + uri.hostname == config.gitlab.host && + (uri.port.blank? || uri.port == config.gitlab.port)) end def internal_shell?(uri) diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index d7134d3d25a..8784f733eda 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -184,7 +184,7 @@ describe ApplicationController do expect(response).to have_gitlab_http_status(404) end - it 'redirects to login page via authenticate_user! if not authenticated' do + it 'redirects to login page if not authenticated' do get :index expect(response).to redirect_to new_user_session_path diff --git a/spec/controllers/concerns/internal_redirect_spec.rb b/spec/controllers/concerns/internal_redirect_spec.rb index da68c8c8697..e5e50cfd55e 100644 --- a/spec/controllers/concerns/internal_redirect_spec.rb +++ b/spec/controllers/concerns/internal_redirect_spec.rb @@ -19,7 +19,8 @@ describe InternalRedirect do [ 'Hello world', '//example.com/hello/world', - 'https://example.com/hello/world' + 'https://example.com/hello/world', + "not-starting-with-a-slash\n/starting/with/slash" ] end diff --git a/spec/controllers/concerns/lfs_request_spec.rb b/spec/controllers/concerns/lfs_request_spec.rb index cb8c0b8f71c..823b9a50434 100644 --- a/spec/controllers/concerns/lfs_request_spec.rb +++ b/spec/controllers/concerns/lfs_request_spec.rb @@ -16,13 +16,17 @@ describe LfsRequest do end def project - @project ||= Project.find(params[:id]) + @project ||= Project.find_by(id: params[:id]) end def download_request? true end + def upload_request? + false + end + def ci? false end @@ -49,4 +53,41 @@ describe LfsRequest do expect(assigns(:storage_project)).to eq(project) end end + + context 'user is authenticated without access to lfs' do + before do + allow(controller).to receive(:authenticate_user) + allow(controller).to receive(:authentication_result) do + Gitlab::Auth::Result.new + end + end + + context 'with access to the project' do + it 'returns 403' do + get :show, params: { id: project.id } + + expect(response.status).to eq(403) + end + end + + context 'without access to the project' do + context 'project does not exist' do + it 'returns 404' do + get :show, params: { id: 'does not exist' } + + expect(response.status).to eq(404) + end + end + + context 'project is private' do + let(:project) { create(:project, :private) } + + it 'returns 404' do + get :show, params: { id: project.id } + + expect(response.status).to eq(404) + end + end + end + end end diff --git a/spec/controllers/projects/autocomplete_sources_controller_spec.rb b/spec/controllers/projects/autocomplete_sources_controller_spec.rb index a9a058e7e17..f9c8e91d816 100644 --- a/spec/controllers/projects/autocomplete_sources_controller_spec.rb +++ b/spec/controllers/projects/autocomplete_sources_controller_spec.rb @@ -8,6 +8,10 @@ describe Projects::AutocompleteSourcesController do set(:issue) { create(:issue, project: project) } set(:user) { create(:user) } + def members_by_username(username) + json_response.find { |member| member['username'] == username } + end + describe 'GET members' do before do group.add_owner(user) @@ -17,22 +21,21 @@ describe Projects::AutocompleteSourcesController do it 'returns an array of member object' do get :members, format: :json, params: { namespace_id: group.path, project_id: project.path, type: issue.class.name, type_id: issue.id } - all = json_response.find {|member| member["username"] == 'all'} - the_group = json_response.find {|member| member["username"] == group.full_path} - the_user = json_response.find {|member| member["username"] == user.username} - - expect(all.symbolize_keys).to include(username: 'all', - name: 'All Project and Group Members', - count: 1) - - expect(the_group.symbolize_keys).to include(type: group.class.name, - name: group.full_name, - avatar_url: group.avatar_url, - count: 1) - - expect(the_user.symbolize_keys).to include(type: user.class.name, - name: user.name, - avatar_url: user.avatar_url) + expect(members_by_username('all').symbolize_keys).to include( + username: 'all', + name: 'All Project and Group Members', + count: 1) + + expect(members_by_username(group.full_path).symbolize_keys).to include( + type: group.class.name, + name: group.full_name, + avatar_url: group.avatar_url, + count: 1) + + expect(members_by_username(user.username).symbolize_keys).to include( + type: user.class.name, + name: user.name, + avatar_url: user.avatar_url) end end diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb index 9c4d6fdcb2a..1977e92e42b 100644 --- a/spec/controllers/projects/commits_controller_spec.rb +++ b/spec/controllers/projects/commits_controller_spec.rb @@ -142,7 +142,7 @@ describe Projects::CommitsController do context 'token authentication' do context 'public project' do - it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do + it_behaves_like 'authenticates sessionless user', :show, :atom, { public: true, ignore_incrementing: true } do before do public_project = create(:project, :repository, :public) @@ -152,7 +152,7 @@ describe Projects::CommitsController do end context 'private project' do - it_behaves_like 'authenticates sessionless user', :show, :atom, public: false do + it_behaves_like 'authenticates sessionless user', :show, :atom, { public: false, ignore_incrementing: true } do before do private_project = create(:project, :repository, :private) private_project.add_maintainer(user) diff --git a/spec/controllers/projects/error_tracking_controller_spec.rb b/spec/controllers/projects/error_tracking_controller_spec.rb index d11ef24ef96..92b63ec96d5 100644 --- a/spec/controllers/projects/error_tracking_controller_spec.rb +++ b/spec/controllers/projects/error_tracking_controller_spec.rb @@ -146,7 +146,7 @@ describe Projects::ErrorTrackingController do it 'redirects to sign-in page' do post :list_projects, params: list_projects_params - expect(response).to have_gitlab_http_status(:unauthorized) + expect(response).to have_gitlab_http_status(:redirect) end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index ad57c29850b..ff41f46fc3c 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1386,7 +1386,7 @@ describe Projects::IssuesController do context 'private project with token authentication' do let(:private_project) { create(:project, :private) } - it_behaves_like 'authenticates sessionless user', :index, :atom do + it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true do before do default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) @@ -1394,7 +1394,7 @@ describe Projects::IssuesController do end end - it_behaves_like 'authenticates sessionless user', :calendar, :ics do + it_behaves_like 'authenticates sessionless user', :calendar, :ics, ignore_incrementing: true do before do default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) diff --git a/spec/controllers/projects/tags_controller_spec.rb b/spec/controllers/projects/tags_controller_spec.rb index b99b5d611fc..f077b4c99fc 100644 --- a/spec/controllers/projects/tags_controller_spec.rb +++ b/spec/controllers/projects/tags_controller_spec.rb @@ -41,7 +41,7 @@ describe Projects::TagsController do context 'private project with token authentication' do let(:private_project) { create(:project, :repository, :private) } - it_behaves_like 'authenticates sessionless user', :index, :atom do + it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true do before do default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index c732caa6160..9cafbc99e79 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -1149,7 +1149,7 @@ describe ProjectsController do context 'private project with token authentication' do let(:private_project) { create(:project, :private) } - it_behaves_like 'authenticates sessionless user', :show, :atom do + it_behaves_like 'authenticates sessionless user', :show, :atom, ignore_incrementing: true do before do default_params.merge!(id: private_project, namespace_id: private_project.namespace) diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index 4fb72eb8737..76d8ad1638b 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -827,7 +827,10 @@ describe 'Pipelines', :js do context 'when project is private' do let(:project) { create(:project, :private, :repository) } - it { expect(page).to have_content 'You need to sign in' } + it 'redirects the user to sign_in and displays the flash alert' do + expect(page).to have_content 'You need to sign in' + expect(page.current_path).to eq("/users/sign_in") + end end end diff --git a/spec/features/projects/tags/user_views_tags_spec.rb b/spec/features/projects/tags/user_views_tags_spec.rb index f344b682715..bc570f502bf 100644 --- a/spec/features/projects/tags/user_views_tags_spec.rb +++ b/spec/features/projects/tags/user_views_tags_spec.rb @@ -15,7 +15,7 @@ describe 'User views tags', :feature do it do visit project_tags_path(project, format: :atom) - expect(page).to have_gitlab_http_status(401) + expect(page.current_path).to eq("/users/sign_in") end end diff --git a/spec/finders/labels_finder_spec.rb b/spec/finders/labels_finder_spec.rb index ba41ded112a..024bfe4d97b 100644 --- a/spec/finders/labels_finder_spec.rb +++ b/spec/finders/labels_finder_spec.rb @@ -127,6 +127,88 @@ describe LabelsFinder do end end end + context 'when including labels from group projects with limited visibility' do + let(:finder) { described_class.new(user, group_id: group_4.id) } + let(:group_4) { create(:group) } + let(:limited_visibility_project) { create(:project, :public, group: group_4) } + let(:visible_project) { create(:project, :public, group: group_4) } + let!(:group_label_1) { create(:group_label, group: group_4) } + let!(:limited_visibility_label) { create(:label, project: limited_visibility_project) } + let!(:visible_label) { create(:label, project: visible_project) } + + shared_examples 'with full visibility' do + it 'returns all projects labels' do + expect(finder.execute).to eq [group_label_1, limited_visibility_label, visible_label] + end + end + + shared_examples 'with limited visibility' do + it 'returns only authorized projects labels' do + expect(finder.execute).to eq [group_label_1, visible_label] + end + end + + context 'when merge requests and issues are not visible for non members' do + before do + limited_visibility_project.project_feature.update!( + merge_requests_access_level: ProjectFeature::PRIVATE, + issues_access_level: ProjectFeature::PRIVATE + ) + end + + context 'when user is not a group member' do + it_behaves_like 'with limited visibility' + end + + context 'when user is a group member' do + before do + group_4.add_developer(user) + end + + it_behaves_like 'with full visibility' + end + end + + context 'when merge requests are not visible for non members' do + before do + limited_visibility_project.project_feature.update!( + merge_requests_access_level: ProjectFeature::PRIVATE + ) + end + + context 'when user is not a group member' do + it_behaves_like 'with full visibility' + end + + context 'when user is a group member' do + before do + group_4.add_developer(user) + end + + it_behaves_like 'with full visibility' + end + end + + context 'when issues are not visible for non members' do + before do + limited_visibility_project.project_feature.update!( + issues_access_level: ProjectFeature::PRIVATE + ) + end + + context 'when user is not a group member' do + it_behaves_like 'with full visibility' + end + + context 'when user is a group member' do + before do + group_4.add_developer(user) + end + + it_behaves_like 'with full visibility' + end + end + end context 'filtering by project_id' do context 'when include_ancestor_groups is true' do diff --git a/spec/fixtures/api/graphql/recursive-introspection.graphql b/spec/fixtures/api/graphql/recursive-introspection.graphql new file mode 100644 index 00000000000..db970bb14b6 --- /dev/null +++ b/spec/fixtures/api/graphql/recursive-introspection.graphql @@ -0,0 +1,57 @@ +query allSchemaTypes { + __schema { + types { + fields { + type{ + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + type { + fields { + name + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } +}
\ No newline at end of file diff --git a/spec/fixtures/api/graphql/recursive-query.graphql b/spec/fixtures/api/graphql/recursive-query.graphql new file mode 100644 index 00000000000..d1616c4de6e --- /dev/null +++ b/spec/fixtures/api/graphql/recursive-query.graphql @@ -0,0 +1,47 @@ +{ + project(fullPath: "gitlab-org/gitlab-ce") { + group { + projects { + edges { + node { + group { + projects { + edges { + node { + group { + projects { + edges { + node { + group { + projects { + edges { + node { + group { + projects { + edges { + node { + group { + description + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } +} diff --git a/spec/fixtures/api/graphql/small-recursive-introspection.graphql b/spec/fixtures/api/graphql/small-recursive-introspection.graphql new file mode 100644 index 00000000000..1025043b474 --- /dev/null +++ b/spec/fixtures/api/graphql/small-recursive-introspection.graphql @@ -0,0 +1,15 @@ +query allSchemaTypes { + __schema { + types { + fields { + type { + fields { + type { + name + } + } + } + } + } + } +} diff --git a/spec/frontend/project_find_file_spec.js b/spec/frontend/project_find_file_spec.js index 8102033139f..e60f9f62747 100644 --- a/spec/frontend/project_find_file_spec.js +++ b/spec/frontend/project_find_file_spec.js @@ -3,6 +3,9 @@ import $ from 'jquery'; import ProjectFindFile from '~/project_find_file'; import axios from '~/lib/utils/axios_utils'; import { TEST_HOST } from 'helpers/test_constants'; +import sanitize from 'sanitize-html'; + +jest.mock('sanitize-html', () => jest.fn(val => val)); const BLOB_URL_TEMPLATE = `${TEST_HOST}/namespace/project/blob/master`; const FILE_FIND_URL = `${TEST_HOST}/namespace/project/files/master?format=json`; @@ -38,31 +41,31 @@ describe('ProjectFindFile', () => { href: el.querySelector('a').href, })); + const files = [ + 'fileA.txt', + 'fileB.txt', + 'fi#leC.txt', + 'folderA/fileD.txt', + 'folder#B/fileE.txt', + 'folde?rC/fil#F.txt', + ]; + beforeEach(() => { // Create a mock adapter for stubbing axios API requests mock = new MockAdapter(axios); element = $(TEMPLATE); + mock.onGet(FILE_FIND_URL).replyOnce(200, files); + getProjectFindFileInstance(); // This triggers a load / axios call + subsequent render in the constructor }); afterEach(() => { // Reset the mock adapter mock.restore(); + sanitize.mockClear(); }); it('loads and renders elements from remote server', done => { - const files = [ - 'fileA.txt', - 'fileB.txt', - 'fi#leC.txt', - 'folderA/fileD.txt', - 'folder#B/fileE.txt', - 'folde?rC/fil#F.txt', - ]; - mock.onGet(FILE_FIND_URL).replyOnce(200, files); - - getProjectFindFileInstance(); // This triggers a load / axios call + subsequent render in the constructor - setImmediate(() => { expect(findFiles()).toEqual( files.map(text => ({ @@ -74,4 +77,14 @@ describe('ProjectFindFile', () => { done(); }); }); + + it('sanitizes search text', done => { + const searchText = element.find('.file-finder-input').val(); + + setImmediate(() => { + expect(sanitize).toHaveBeenCalledTimes(1); + expect(sanitize).toHaveBeenCalledWith(searchText); + done(); + }); + }); }); diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index f6e1720e113..f0bf547f778 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -1,18 +1,18 @@ require 'spec_helper' describe MarkupHelper do - let!(:project) { create(:project, :repository) } - - let(:user) { create(:user, username: 'gfm') } - let(:commit) { project.commit } - let(:issue) { create(:issue, project: project) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - let(:snippet) { create(:project_snippet, project: project) } - - before do - # Ensure the generated reference links aren't redacted + set(:project) { create(:project, :repository) } + set(:user) do + user = create(:user, username: 'gfm') project.add_maintainer(user) + user + end + set(:issue) { create(:issue, project: project) } + set(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + set(:snippet) { create(:project_snippet, project: project) } + let(:commit) { project.commit } + before do # Helper expects a @project instance variable helper.instance_variable_set(:@project, project) @@ -42,8 +42,8 @@ describe MarkupHelper do describe "override default project" do let(:actual) { issue.to_reference } - let(:second_project) { create(:project, :public) } - let(:second_issue) { create(:issue, project: second_project) } + set(:second_project) { create(:project, :public) } + set(:second_issue) { create(:issue, project: second_project) } it 'links to the issue' do expected = urls.project_issue_path(second_project, second_issue) @@ -53,7 +53,7 @@ describe MarkupHelper do describe 'uploads' do let(:text) { "![ImageTest](/uploads/test.png)" } - let(:group) { create(:group) } + set(:group) { create(:group) } subject { helper.markdown(text) } @@ -75,7 +75,7 @@ describe MarkupHelper do end describe "with a group in the context" do - let(:project_in_group) { create(:project, group: group) } + set(:project_in_group) { create(:project, group: group) } before do helper.instance_variable_set(:@group, group) @@ -233,38 +233,48 @@ describe MarkupHelper do end describe '#render_wiki_content' do + let(:wiki) { double('WikiPage', path: "file.#{extension}") } + let(:context) do + { + pipeline: :wiki, project: project, project_wiki: wiki, + page_slug: 'nested/page', issuable_state_filter_enabled: true + } + end + before do - @wiki = double('WikiPage') - allow(@wiki).to receive(:content).and_return('wiki content') - allow(@wiki).to receive(:slug).and_return('nested/page') - helper.instance_variable_set(:@project_wiki, @wiki) + expect(wiki).to receive(:content).and_return('wiki content') + expect(wiki).to receive(:slug).and_return('nested/page') + helper.instance_variable_set(:@project_wiki, wiki) end - it "uses Wiki pipeline for markdown files" do - allow(@wiki).to receive(:format).and_return(:markdown) + context 'when file is Markdown' do + let(:extension) { 'md' } - expect(helper).to receive(:markdown_unsafe).with('wiki content', - pipeline: :wiki, project: project, project_wiki: @wiki, page_slug: "nested/page", - issuable_state_filter_enabled: true) + it 'renders using #markdown_unsafe helper method' do + expect(helper).to receive(:markdown_unsafe).with('wiki content', context) - helper.render_wiki_content(@wiki) + helper.render_wiki_content(wiki) + end end - it "uses Asciidoctor for asciidoc files" do - allow(@wiki).to receive(:format).and_return(:asciidoc) + context 'when file is Asciidoc' do + let(:extension) { 'adoc' } - expect(helper).to receive(:asciidoc_unsafe).with('wiki content') + it 'renders using Gitlab::Asciidoc' do + expect(Gitlab::Asciidoc).to receive(:render) - helper.render_wiki_content(@wiki) + helper.render_wiki_content(wiki) + end end - it "uses the Gollum renderer for all other file types" do - allow(@wiki).to receive(:format).and_return(:rdoc) - formatted_content_stub = double('formatted_content') - expect(formatted_content_stub).to receive(:html_safe) - allow(@wiki).to receive(:formatted_content).and_return(formatted_content_stub) + context 'any other format' do + let(:extension) { 'foo' } - helper.render_wiki_content(@wiki) + it 'renders all other formats using Gitlab::OtherMarkup' do + expect(Gitlab::OtherMarkup).to receive(:render) + + helper.render_wiki_content(wiki) + end end end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 0e66e959b24..2bdc70205b9 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -62,6 +62,22 @@ describe Gitlab::UrlBlocker do expect { subject }.to raise_error(described_class::BlockedUrlError) end end + + context 'when domain is too long' do + let(:import_url) { 'https://example' + 'a' * 1024 + '.com' } + + it 'raises an error' do + expect { subject }.to raise_error(ArgumentError) + end + end + + context 'when scheme is missing' do + let(:import_url) { '//example.org/path' } + + it 'raises and error' do + expect { subject }.to raise_error(described_class::BlockedUrlError) + end + end end context 'when the URL hostname is an IP address' do @@ -83,6 +99,32 @@ describe Gitlab::UrlBlocker do end end + context 'disabled require_absolute' do + subject { described_class.validate!(import_url, require_absolute: false) } + + context 'with scheme and hostname' do + let(:import_url) { 'https://example.org/path' } + + before do + stub_dns(import_url, ip_address: '93.184.216.34') + end + + it_behaves_like 'validates URI and hostname' do + let(:expected_uri) { 'https://93.184.216.34/path' } + let(:expected_hostname) { 'example.org' } + end + end + + context 'without scheme' do + let(:import_url) { '//93.184.216.34/path' } + + it_behaves_like 'validates URI and hostname' do + let(:expected_uri) { import_url } + let(:expected_hostname) { nil } + end + end + end + context 'disabled DNS rebinding protection' do subject { described_class.validate!(import_url, dns_rebind_protection: false) } @@ -608,6 +650,21 @@ describe Gitlab::UrlBlocker do end end + context 'when require_absolute is false' do + it 'allows paths' do + expect(described_class.blocked_url?('/foo/foo.bar', require_absolute: false)).to be false + end + + it 'allows absolute urls without paths' do + expect(described_class.blocked_url?('http://example.com', require_absolute: false)).to be false + end + + it 'paths must begin with a slash' do + expect(described_class.blocked_url?('foo/foo.bar', require_absolute: false)).to be true + expect(described_class.blocked_url?('', require_absolute: false)).to be true + end + end + it 'blocks urls with invalid ip address' do stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index d12f9b9100a..855ce9cb4f9 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -17,6 +17,7 @@ describe ApplicationSetting do let(:http) { 'http://example.com' } let(:https) { 'https://example.com' } let(:ftp) { 'ftp://example.com' } + let(:javascript) { 'javascript:alert(window.opener.document.location)' } it { is_expected.to allow_value(nil).for(:home_page_url) } it { is_expected.to allow_value(http).for(:home_page_url) } @@ -52,6 +53,11 @@ describe ApplicationSetting do it { is_expected.to allow_value(http).for(:static_objects_external_storage_url) } it { is_expected.to allow_value(https).for(:static_objects_external_storage_url) } + it { is_expected.to allow_value(http).for(:grafana_url) } + it { is_expected.to allow_value(https).for(:grafana_url) } + it { is_expected.not_to allow_value(ftp).for(:grafana_url) } + it { is_expected.not_to allow_value(javascript).for(:grafana_url) } + context "when user accepted let's encrypt terms of service" do before do setting.update(lets_encrypt_terms_of_service_accepted: true) diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 2ecbe548520..120ba67f328 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -227,6 +227,14 @@ describe Milestone do end end + describe '#to_ability_name' do + it 'returns milestone' do + milestone = build(:milestone) + + expect(milestone.to_ability_name).to eq('milestone') + end + end + describe '.search' do let(:milestone) { create(:milestone, title: 'foo', description: 'bar') } diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 66e3c6d5e9d..7c6de1e1e06 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -346,6 +346,63 @@ describe Note do expect(label_note.cross_reference?).to be_falsy end end + + context 'when system note metadata is not present' do + let(:note) { build(:note, :system) } + + before do + allow(note).to receive(:system_note_metadata).and_return(nil) + end + + it 'delegates to the system note service' do + expect(SystemNoteService).to receive(:cross_reference?).with(note.note) + + note.cross_reference? + end + end + + context 'with a system note' do + let(:issue) { create(:issue, project: create(:project, :repository)) } + let(:note) { create(:system_note, note: "test", noteable: issue, project: issue.project) } + + shared_examples 'system_note_metadata includes note action' do + it 'delegates to the cross-reference regex' do + expect(note).to receive(:matches_cross_reference_regex?) + + note.cross_reference? + end + end + + context 'with :label action' do + let!(:metadata) {create(:system_note_metadata, note: note, action: :label)} + + it_behaves_like 'system_note_metadata includes note action' + + it { expect(note.cross_reference?).to be_falsy } + + context 'with cross reference label note' do + let(:label) { create(:label, project: issue.project)} + let(:note) { create(:system_note, note: "added #{label.to_reference} label", noteable: issue, project: issue.project) } + + it { expect(note.cross_reference?).to be_truthy } + end + end + + context 'with :milestone action' do + let!(:metadata) {create(:system_note_metadata, note: note, action: :milestone)} + + it_behaves_like 'system_note_metadata includes note action' + + it { expect(note.cross_reference?).to be_falsy } + + context 'with cross reference milestone note' do + let(:milestone) { create(:milestone, project: issue.project)} + let(:note) { create(:system_note, note: "added #{milestone.to_reference} milestone", noteable: issue, project: issue.project) } + + it { expect(note.cross_reference?).to be_truthy } + end + end + end end describe 'clear_blank_line_code!' do @@ -545,24 +602,30 @@ describe Note do end describe '#to_ability_name' do - it 'returns snippet for a project snippet note' do - expect(build(:note_on_project_snippet).to_ability_name).to eq('project_snippet') + it 'returns note' do + expect(build(:note).to_ability_name).to eq('note') + end + end + + describe '#noteable_ability_name' do + it 'returns project_snippet for a project snippet note' do + expect(build(:note_on_project_snippet).noteable_ability_name).to eq('project_snippet') end it 'returns personal_snippet for a personal snippet note' do - expect(build(:note_on_personal_snippet).to_ability_name).to eq('personal_snippet') + expect(build(:note_on_personal_snippet).noteable_ability_name).to eq('personal_snippet') end it 'returns merge_request for an MR note' do - expect(build(:note_on_merge_request).to_ability_name).to eq('merge_request') + expect(build(:note_on_merge_request).noteable_ability_name).to eq('merge_request') end it 'returns issue for an issue note' do - expect(build(:note_on_issue).to_ability_name).to eq('issue') + expect(build(:note_on_issue).noteable_ability_name).to eq('issue') end - it 'returns issue for a commit note' do - expect(build(:note_on_commit).to_ability_name).to eq('commit') + it 'returns commit for a commit note' do + expect(build(:note_on_commit).noteable_ability_name).to eq('commit') end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7fe48e66def..92648cddb24 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3341,7 +3341,7 @@ describe Project do end end - describe '.ids_with_milestone_available_for' do + describe '.ids_with_issuables_available_for' do let!(:user) { create(:user) } it 'returns project ids with milestones available for user' do @@ -3351,7 +3351,7 @@ describe Project do project_4 = create(:project, :public) project_4.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE ) - project_ids = described_class.ids_with_milestone_available_for(user).pluck(:id) + project_ids = described_class.ids_with_issuables_available_for(user).pluck(:id) expect(project_ids).to include(project_2.id, project_3.id) expect(project_ids).not_to include(project_1.id, project_4.id) @@ -4372,6 +4372,14 @@ describe Project do end end + describe '#to_ability_name' do + it 'returns project' do + project = build(:project_empty_repo) + + expect(project.to_ability_name).to eq('project') + end + end + describe '#execute_hooks' do let(:data) { { ref: 'refs/heads/master', data: 'data' } } it 'executes active projects hooks with the specified scope' do diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 18c62c917dc..9014276dcf8 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -439,6 +439,23 @@ describe WikiPage do end end + describe '#path' do + let(:path) { 'mypath.md' } + let(:wiki_page) { instance_double('Gitlab::Git::WikiPage', path: path).as_null_object } + + it 'returns the path when persisted' do + page = described_class.new(wiki, wiki_page, true) + + expect(page.path).to eq(path) + end + + it 'returns nil when not persisted' do + page = described_class.new(wiki, wiki_page, false) + + expect(page.path).to be_nil + end + end + describe '#directory' do context 'when the page is at the root directory' do it 'returns an empty string' do diff --git a/spec/policies/commit_policy_spec.rb b/spec/policies/commit_policy_spec.rb index 41f6fb08426..40183f51e9e 100644 --- a/spec/policies/commit_policy_spec.rb +++ b/spec/policies/commit_policy_spec.rb @@ -8,28 +8,42 @@ describe CommitPolicy do let(:commit) { project.repository.head_commit } let(:policy) { described_class.new(user, commit) } + shared_examples 'can read commit and create a note' do + it 'can read commit' do + expect(policy).to be_allowed(:read_commit) + end + + it 'can create a note' do + expect(policy).to be_allowed(:create_note) + end + end + + shared_examples 'cannot read commit nor create a note' do + it 'can not read commit' do + expect(policy).to be_disallowed(:read_commit) + end + + it 'can not create a note' do + expect(policy).to be_disallowed(:create_note) + end + end + context 'when project is public' do let(:project) { create(:project, :public, :repository) } - it 'can read commit and create a note' do - expect(policy).to be_allowed(:read_commit) - end + it_behaves_like 'can read commit and create a note' context 'when repository access level is private' do let(:project) { create(:project, :public, :repository, :repository_private) } - it 'can not read commit and create a note' do - expect(policy).to be_disallowed(:read_commit) - end + it_behaves_like 'cannot read commit nor create a note' context 'when the user is a project member' do before do project.add_developer(user) end - it 'can read commit and create a note' do - expect(policy).to be_allowed(:read_commit) - end + it_behaves_like 'can read commit and create a note' end end end @@ -37,9 +51,7 @@ describe CommitPolicy do context 'when project is private' do let(:project) { create(:project, :private, :repository) } - it 'can not read commit and create a note' do - expect(policy).to be_disallowed(:read_commit) - end + it_behaves_like 'cannot read commit nor create a note' context 'when the user is a project member' do before do @@ -50,6 +62,18 @@ describe CommitPolicy do expect(policy).to be_allowed(:read_commit) end end + + context 'when the user is a guest' do + before do + project.add_guest(user) + end + + it_behaves_like 'cannot read commit nor create a note' + + it 'cannot download code' do + expect(policy).to be_disallowed(:download_code) + end + end end end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index be55d94daec..ede07a5f6a2 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -335,6 +335,88 @@ describe GroupPolicy do end end + context 'transfer_projects' do + shared_examples_for 'allowed to transfer projects' do + before do + group.update(project_creation_level: project_creation_level) + end + + it { is_expected.to be_allowed(:transfer_projects) } + end + + shared_examples_for 'not allowed to transfer projects' do + before do + group.update(project_creation_level: project_creation_level) + end + + it { is_expected.to be_disallowed(:transfer_projects) } + end + + context 'reporter' do + let(:current_user) { reporter } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + + context 'developer' do + let(:current_user) { developer } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + + context 'owner' do + let(:current_user) { owner } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + end + context "create_projects" do context 'when group has no project creation level set' do let(:group) { create(:group, project_creation_level: nil) } diff --git a/spec/policies/namespace_policy_spec.rb b/spec/policies/namespace_policy_spec.rb index 216aaae70ee..909c17fe8b5 100644 --- a/spec/policies/namespace_policy_spec.rb +++ b/spec/policies/namespace_policy_spec.rb @@ -6,7 +6,7 @@ describe NamespacePolicy do let(:admin) { create(:admin) } let(:namespace) { create(:namespace, owner: owner) } - let(:owner_permissions) { [:create_projects, :admin_namespace, :read_namespace, :read_statistics] } + let(:owner_permissions) { [:create_projects, :admin_namespace, :read_namespace, :read_statistics, :transfer_projects] } subject { described_class.new(current_user, namespace) } @@ -31,6 +31,7 @@ describe NamespacePolicy do let(:owner) { create(:user, projects_limit: 0) } it { is_expected.to be_disallowed(:create_projects) } + it { is_expected.to be_disallowed(:transfer_projects) } end end diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 1be8883bd3c..deeab82a80d 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -322,7 +322,7 @@ describe API::CommitStatuses do it 'responds with bad request status and validation errors' do expect(response).to have_gitlab_http_status(400) expect(json_response['message']['target_url']) - .to include 'is blocked: Only allowed schemes are http, https' + .to include 'is blocked: must be absolute' end end diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb index e1eb7c7f738..1e799a0a42a 100644 --- a/spec/requests/api/graphql/gitlab_schema_spec.rb +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -13,7 +13,7 @@ describe 'GitlabSchema configurations' do subject - expect(graphql_errors.flatten.first['message']).to include('which exceeds max complexity of 1') + expect_graphql_errors_to_include /which exceeds max complexity of 1/ end end end @@ -21,12 +21,11 @@ describe 'GitlabSchema configurations' do describe '#max_depth' do context 'when query depth is too high' do it 'shows error' do - errors = { "message" => "Query has depth of 2, which exceeds max depth of 1" } allow(GitlabSchema).to receive(:max_query_depth).and_return 1 subject - expect(graphql_errors.flatten).to include(errors) + expect_graphql_errors_to_include /exceeds max depth/ end end @@ -36,7 +35,42 @@ describe 'GitlabSchema configurations' do subject - expect(Array.wrap(graphql_errors).compact).to be_empty + expect_graphql_errors_to_be_empty + end + end + end + end + + context 'depth, complexity and recursion checking' do + context 'unauthenticated recursive queries' do + context 'a not-quite-recursive-enough introspective query' do + it 'succeeds' do + query = File.read(Rails.root.join('spec/fixtures/api/graphql/small-recursive-introspection.graphql')) + + post_graphql(query, current_user: nil) + + expect_graphql_errors_to_be_empty + end + end + + context 'a deep but simple recursive introspective query' do + it 'fails due to recursion' do + query = File.read(Rails.root.join('spec/fixtures/api/graphql/recursive-introspection.graphql')) + + post_graphql(query, current_user: nil) + + expect_graphql_errors_to_include [/Recursive query/] + end + end + + context 'a deep recursive non-introspective query' do + it 'fails due to recursion, complexity and depth' do + allow(GitlabSchema).to receive(:max_query_complexity).and_return 1 + query = File.read(Rails.root.join('spec/fixtures/api/graphql/recursive-query.graphql')) + + post_graphql(query, current_user: nil) + + expect_graphql_errors_to_include [/Recursive query/, /exceeds max complexity/, /exceeds max depth/] end end end @@ -86,7 +120,7 @@ describe 'GitlabSchema configurations' do # Expect errors for each query expect(graphql_errors.size).to eq(3) graphql_errors.each do |single_query_errors| - expect(single_query_errors.first['message']).to include('which exceeds max complexity of 4') + expect_graphql_errors_to_include(/which exceeds max complexity of 4/) end end end @@ -103,12 +137,12 @@ describe 'GitlabSchema configurations' do end context 'when IntrospectionQuery' do - it 'is not too complex' do + it 'is not too complex nor recursive' do query = File.read(Rails.root.join('spec/fixtures/api/graphql/introspection.graphql')) post_graphql(query, current_user: nil) - expect(graphql_errors).to be_nil + expect_graphql_errors_to_be_empty end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 8179da2f97c..05160a33e61 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1737,6 +1737,38 @@ describe API::MergeRequests do expect(json_response['state']).to eq('closed') expect(json_response['force_remove_source_branch']).to be_truthy end + + context 'with a merge request across forks' do + let(:fork_owner) { create(:user) } + let(:source_project) { fork_project(project, fork_owner) } + let(:target_project) { project } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: target_project, + source_branch: 'fixes', + merge_params: { 'force_remove_source_branch' => false }) + end + + it 'is true for an authorized user' do + put api("/projects/#{target_project.id}/merge_requests/#{merge_request.iid}", fork_owner), params: { state_event: 'close', remove_source_branch: true } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['state']).to eq('closed') + expect(json_response['force_remove_source_branch']).to be true + end + + it 'is false for an unauthorized user' do + expect do + put api("/projects/#{target_project.id}/merge_requests/#{merge_request.iid}", target_project.owner), params: { state_event: 'close', remove_source_branch: true } + end.not_to change { merge_request.reload.merge_params } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['state']).to eq('closed') + expect(json_response['force_remove_source_branch']).to be false + end + end end context "to close a MR" do diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 2d8ef9c06dc..99d2a68ef53 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -2648,6 +2648,22 @@ describe API::Projects do expect(response).to have_gitlab_http_status(400) end end + + context 'when authenticated as developer' do + before do + group.add_developer(user) + end + + context 'target namespace allows developers to create projects' do + let(:group) { create(:group, project_creation_level: ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) } + + it 'fails transferring the project to the target namespace' do + put api("/projects/#{project.id}/transfer", user), params: { namespace: group.id } + + expect(response).to have_gitlab_http_status(400) + end + end + end end it_behaves_like 'custom attributes endpoints', 'projects' do diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index a409f21a7e4..0a6bcb1badc 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -51,7 +51,7 @@ describe AutoMerge::BaseService do expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18') expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) - expect(merge_request.merge_params['squash']).to eq(false) + expect(merge_request.squash).to eq(false) expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md') end end @@ -108,7 +108,6 @@ describe AutoMerge::BaseService do 'commit_message' => "Merge branch 'patch-12' into 'master'", 'sha' => "200fcc9c260f7219eaf0daba87d818f0922c5b18", 'should_remove_source_branch' => false, - 'squash' => false, 'squash_commit_message' => "Update README.md" } end diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 931b52470c4..ccbb4e7c30d 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -59,8 +59,9 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do it 'sets the params, merge_user, and flag' do expect(merge_request).to be_valid expect(merge_request.merge_when_pipeline_succeeds).to be_truthy - expect(merge_request.merge_params).to include commit_message: 'Awesome message' + expect(merge_request.merge_params).to include 'commit_message' => 'Awesome message' expect(merge_request.merge_user).to be user + expect(merge_request.auto_merge_strategy).to eq AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS end it 'creates a system note' do @@ -73,7 +74,7 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do end context 'already approved' do - let(:service) { described_class.new(project, user, new_key: true) } + let(:service) { described_class.new(project, user, should_remove_source_branch: true) } let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } before do @@ -90,7 +91,7 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do expect(SystemNoteService).not_to receive(:merge_when_pipeline_succeeds) service.execute(mr_merge_if_green_enabled) - expect(mr_merge_if_green_enabled.merge_params).to have_key(:new_key) + expect(mr_merge_if_green_enabled.merge_params).to have_key('should_remove_source_branch') end end end diff --git a/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb b/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb new file mode 100644 index 00000000000..5b653aa331c --- /dev/null +++ b/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb @@ -0,0 +1,70 @@ +require 'spec_helper' + +describe MergeRequests::AssignsMergeParams do + it 'raises an error when used from an instance that does not respond to #current_user' do + define_class = -> { Class.new { include MergeRequests::AssignsMergeParams }.new } + + expect { define_class.call }.to raise_error %r{can not be included in (.*) without implementing #current_user} + end + + describe '#assign_allowed_merge_params' do + let(:merge_request) { build(:merge_request) } + + let(:params) do + { commit_message: 'Commit Message', + 'should_remove_source_branch' => true, + unknown_symbol: 'Unknown symbol', + 'unknown_string' => 'Unknown String' } + end + + subject(:merge_request_service) do + Class.new do + attr_accessor :current_user + + include MergeRequests::AssignsMergeParams + + def initialize(user) + @current_user = user + end + end + end + + it 'only assigns known parameters to the merge request' do + service = merge_request_service.new(merge_request.author) + + service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.merge_params).to eq('commit_message' => 'Commit Message', 'should_remove_source_branch' => true) + end + + it 'returns a hash without the known merge params' do + service = merge_request_service.new(merge_request.author) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(result).to eq({ 'unknown_symbol' => 'Unknown symbol', 'unknown_string' => 'Unknown String' }) + end + + context 'the force_remove_source_branch param' do + let(:params) { { force_remove_source_branch: true } } + + it 'assigns the param if the user is allowed to do that' do + service = merge_request_service.new(merge_request.author) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.force_remove_source_branch?).to be true + expect(result).to be_empty + end + + it 'only removes the param if the user is not allowed to do that' do + service = merge_request_service.new(build(:user)) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.force_remove_source_branch?).to be_falsy + expect(result).to be_empty + end + end + end +end diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb index 730fccc599e..a272a604184 100644 --- a/spec/services/error_tracking/list_projects_service_spec.rb +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -50,6 +50,19 @@ describe ErrorTracking::ListProjectsService do end end + context 'masked param token' do + let(:params) { ActionController::Parameters.new(token: "*********", api_host: new_api_host) } + + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) + .and_return({ projects: [] }) + end + + it 'uses database token' do + expect { subject.execute }.not_to change { error_tracking_setting.token } + end + end + context 'sentry client raises exception' do context 'Sentry::Client::Error' do before do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index d546a092680..68e53553043 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -83,8 +83,9 @@ describe MergeRequests::BuildService do expect(merge_request.force_remove_source_branch?).to be_falsey end - context 'with force_remove_source_branch parameter' do + context 'with force_remove_source_branch parameter when the user is authorized' do let(:mr_params) { params.merge(force_remove_source_branch: '1') } + let(:source_project) { fork_project(project, user) } let(:merge_request) { described_class.new(project, user, mr_params).execute } it 'assigns force_remove_source_branch' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 9688e02d6ac..bcfd990573c 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -644,5 +644,29 @@ describe MergeRequests::UpdateService, :mailer do expect(merge_request.allow_collaboration).to be_truthy end end + + context 'updating `force_remove_source_branch`' do + let(:target_project) { create(:project, :repository, :public) } + let(:source_project) { fork_project(target_project, nil, repository: true) } + let(:user) { target_project.owner } + let(:merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'fixes', + target_project: target_project) + end + + it "cannot be done by members of the target project when they don't have access" do + expect { update_merge_request(force_remove_source_branch: true) } + .not_to change { merge_request.reload.force_remove_source_branch? }.from(nil) + end + + it 'can be done by members of the target project if they can push to the source project' do + source_project.add_developer(user) + + expect { update_merge_request(force_remove_source_branch: true) } + .to change { merge_request.reload.force_remove_source_branch? }.from(nil).to(true) + end + end end end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 7e765659b9d..f1e6116fe3f 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -145,6 +145,27 @@ describe Projects::Operations::UpdateService do end end + context 'with masked param token' do + let(:params) do + { + error_tracking_setting_attributes: { + enabled: false, + token: '*' * 8 + } + } + end + + before do + create(:project_error_tracking_setting, project: project, token: 'token') + end + + it 'does not update token' do + expect(result[:status]).to eq(:success) + + expect(project.error_tracking_setting.token).to eq('token') + end + end + context 'with invalid parameters' do let(:params) { {} } diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index 4def83513a4..239d28557ee 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -57,4 +57,108 @@ describe Projects::ParticipantsService do end end end + + describe '#project_members' do + subject(:usernames) { service.project_members.map { |member| member[:username] } } + + context 'when there is a project in group namespace' do + set(:public_group) { create(:group, :public) } + set(:public_project) { create(:project, :public, namespace: public_group)} + + set(:public_group_owner) { create(:user) } + + let(:service) { described_class.new(public_project, create(:user)) } + + before do + public_group.add_owner(public_group_owner) + end + + it 'returns members of a group' do + expect(usernames).to include(public_group_owner.username) + end + end + + context 'when there is a private group and a public project' do + set(:public_group) { create(:group, :public) } + set(:private_group) { create(:group, :private, :nested) } + set(:public_project) { create(:project, :public, namespace: public_group)} + + set(:project_issue) { create(:issue, project: public_project)} + + set(:public_group_owner) { create(:user) } + set(:private_group_member) { create(:user) } + set(:public_project_maintainer) { create(:user) } + set(:private_group_owner) { create(:user) } + + set(:group_ancestor_owner) { create(:user) } + + before(:context) do + public_group.add_owner public_group_owner + private_group.add_developer private_group_member + public_project.add_maintainer public_project_maintainer + + private_group.add_owner private_group_owner + private_group.parent.add_owner group_ancestor_owner + end + + context 'when the private group is invited to the public project' do + before(:context) do + create(:project_group_link, group: private_group, project: public_project) + end + + context 'when a user who is outside the public project and the private group is signed in' do + let(:service) { described_class.new(public_project, create(:user)) } + + it 'does not return the private group' do + expect(usernames).not_to include(private_group.name) + end + + it 'does not return private group members' do + expect(usernames).not_to include(private_group_member.username) + end + + it 'returns the project maintainer' do + expect(usernames).to include(public_project_maintainer.username) + end + + it 'returns project members from an invited public group' do + invited_public_group = create(:group, :public) + invited_public_group.add_owner create(:user) + + create(:project_group_link, group: invited_public_group, project: public_project) + + expect(usernames).to include(invited_public_group.users.first.username) + end + + it 'does not return ancestors of the private group' do + expect(usernames).not_to include(group_ancestor_owner.username) + end + end + + context 'when private group owner is signed in' do + let(:service) { described_class.new(public_project, private_group_owner) } + + it 'returns private group members' do + expect(usernames).to include(private_group_member.username) + end + + it 'returns ancestors of the the private group' do + expect(usernames).to include(group_ancestor_owner.username) + end + end + + context 'when the namespace owner of the public project is signed in' do + let(:service) { described_class.new(public_project, public_group_owner) } + + it 'returns private group members' do + expect(usernames).to include(private_group_member.username) + end + + it 'does not return members of the ancestral groups of the private group' do + expect(usernames).to include(group_ancestor_owner.username) + end + end + end + end + end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 6b906f9372c..13a490557dd 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -222,6 +222,24 @@ describe Projects::TransferService do it { expect(project.errors[:new_namespace]).to include('Project with same name or path in target namespace already exists') } end + context 'target namespace allows developers to create projects' do + let(:group) { create(:group, project_creation_level: ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) } + + context 'the user is a member of the target namespace with developer permissions' do + subject(:transfer_project_result) { transfer_project(project, user, group) } + + before do + group.add_developer(user) + end + + it 'does not allow project transfer to the target namespace' do + expect(transfer_project_result).to eq false + expect(project.namespace).to eq(user.namespace) + expect(project.errors[:new_namespace]).to include('Transfer failed, please contact an admin.') + end + end + end + def transfer_project(project, user, new_namespace) service = Projects::TransferService.new(project, user) diff --git a/spec/support/controllers/sessionless_auth_controller_shared_examples.rb b/spec/support/controllers/sessionless_auth_controller_shared_examples.rb index b5149a0fcb1..bc95fcd6b88 100644 --- a/spec/support/controllers/sessionless_auth_controller_shared_examples.rb +++ b/spec/support/controllers/sessionless_auth_controller_shared_examples.rb @@ -34,8 +34,15 @@ shared_examples 'authenticates sessionless user' do |path, format, params| context 'when the personal access token has no api scope', unless: params[:public] do it 'does not log the user in' do - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) + # Several instances of where these specs are shared route the request + # through ApplicationController#route_not_found which does not involve + # the usual auth code from Devise, so does not increment the + # :user_unauthenticated_counter + # + unless params[:ignore_incrementing] + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + end personal_access_token.update(scopes: [:read_user]) @@ -84,8 +91,15 @@ shared_examples 'authenticates sessionless user' do |path, format, params| end it "doesn't log the user in otherwise", unless: params[:public] do - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) + # Several instances of where these specs are shared route the request + # through ApplicationController#route_not_found which does not involve + # the usual auth code from Devise, so does not increment the + # :user_unauthenticated_counter + # + unless params[:ignore_incrementing] + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + end get path, params: default_params.merge(private_token: 'token') diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 4d2ad165fd6..6fb1d279456 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -129,6 +129,7 @@ module GraphqlHelpers allow_unlimited_graphql_complexity allow_unlimited_graphql_depth + allow_high_graphql_recursion type = GitlabSchema.types[class_name.to_s] return "" unless type @@ -213,6 +214,23 @@ module GraphqlHelpers end end + def expect_graphql_errors_to_include(regexes_to_match) + raise "No errors. Was expecting to match #{regexes_to_match}" if graphql_errors.nil? || graphql_errors.empty? + + error_messages = flattened_errors.collect { |error_hash| error_hash["message"] } + Array.wrap(regexes_to_match).flatten.each do |regex| + expect(error_messages).to include a_string_matching regex + end + end + + def expect_graphql_errors_to_be_empty + expect(flattened_errors).to be_empty + end + + def flattened_errors + Array.wrap(graphql_errors).flatten.compact + end + # Raises an error if no response is found def graphql_mutation_response(mutation_name) graphql_data.fetch(GraphqlHelpers.fieldnamerize(mutation_name)) @@ -260,6 +278,10 @@ module GraphqlHelpers allow_any_instance_of(GitlabSchema).to receive(:max_depth).and_return nil allow(GitlabSchema).to receive(:max_query_depth).with(any_args).and_return nil end + + def allow_high_graphql_recursion + allow_any_instance_of(Gitlab::Graphql::QueryAnalyzers::RecursionAnalyzer).to receive(:recursion_threshold).and_return 1000 + end end # This warms our schema, doing this as part of loading the helpers to avoid diff --git a/spec/support/shared_examples/controllers/todos_shared_examples.rb b/spec/support/shared_examples/controllers/todos_shared_examples.rb index f3f9abb7da2..914bf506320 100644 --- a/spec/support/shared_examples/controllers/todos_shared_examples.rb +++ b/spec/support/shared_examples/controllers/todos_shared_examples.rb @@ -39,7 +39,7 @@ shared_examples 'todos actions' do post_create end.to change { user.todos.count }.by(0) - expect(response).to have_gitlab_http_status(parent.is_a?(Group) ? 401 : 302) + expect(response).to have_gitlab_http_status(302) end end end diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb index 6927a1f67a1..f57e8f3cfc5 100644 --- a/spec/validators/addressable_url_validator_spec.rb +++ b/spec/validators/addressable_url_validator_spec.rb @@ -349,4 +349,67 @@ describe AddressableUrlValidator do end end end + + context 'when require_absolute is' do + let(:validator) { described_class.new(attributes: [:link_url], require_absolute: require_absolute) } + let(:valid_relative_url) { '/relative/path' } + let(:invalid_relative_url) { 'relative/path' } + let(:absolute_url) { 'https://example.com' } + + context 'true' do + let(:require_absolute) { true } + + it 'prevents valid relative urls' do + badge.link_url = valid_relative_url + + subject + + expect(badge.errors).to be_present + end + + it 'prevents invalid relative urls' do + badge.link_url = invalid_relative_url + + subject + + expect(badge.errors).to be_present + end + + it 'allows absolute urls' do + badge.link_url = absolute_url + + subject + + expect(badge.errors).to be_empty + end + end + + context 'false' do + let(:require_absolute) { false } + + it 'allows valid relative urls' do + badge.link_url = valid_relative_url + + subject + + expect(badge.errors).to be_empty + end + + it 'prevents invalid relative urls' do + badge.link_url = invalid_relative_url + + subject + + expect(badge.errors).to be_present + end + + it 'allows absolute urls' do + badge.link_url = absolute_url + + subject + + expect(badge.errors).to be_empty + end + end + end end |