diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-02-28 20:06:46 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-02-28 20:06:46 +0000 |
commit | a5b13534299fceea9642de04bb1e6a34d9369507 (patch) | |
tree | 94d775338de4582868e8ac5baeecc121602b11cd | |
parent | 557577fba32186d09a11983618aabb5295b287e8 (diff) | |
download | gitlab-ce-a5b13534299fceea9642de04bb1e6a34d9369507.tar.gz |
Add latest changes from gitlab-org/security/gitlab@12-8-stable-ee
45 files changed, 678 insertions, 114 deletions
diff --git a/app/assets/javascripts/error_tracking/components/error_details.vue b/app/assets/javascripts/error_tracking/components/error_details.vue index 7abe3be3e99..047a3f99706 100644 --- a/app/assets/javascripts/error_tracking/components/error_details.vue +++ b/app/assets/javascripts/error_tracking/components/error_details.vue @@ -104,16 +104,6 @@ export default { 'errorStatus', ]), ...mapGetters('details', ['stacktrace']), - reported() { - return sprintf( - __('Reported %{timeAgo} by %{reportedBy}'), - { - reportedBy: `<strong>${this.error.culprit}</strong>`, - timeAgo: this.timeFormatted(this.stacktraceData.date_received), - }, - false, - ); - }, firstReleaseLink() { return `${this.error.externalBaseUrl}/releases/${this.error.firstReleaseShortVersion}`; }, @@ -218,7 +208,17 @@ export default { </gl-alert> <div class="top-area align-items-center justify-content-between py-3"> - <span v-if="!loadingStacktrace && stacktrace" v-html="reported"></span> + <div v-if="!loadingStacktrace && stacktrace" data-qa-selector="reported_text"> + <gl-sprintf :message="__('Reported %{timeAgo} by %{reportedBy}')"> + <template #reportedBy> + <strong>{{ error.culprit }}</strong> + </template> + <template #timeAgo> + {{ timeFormatted(stacktraceData.date_received) }} + </template> + </gl-sprintf> + </div> + <div class="d-inline-flex ml-lg-auto"> <loading-button :label="ignoreBtnLabel" diff --git a/app/controllers/groups/group_links_controller.rb b/app/controllers/groups/group_links_controller.rb index d3360acd245..23daa29ac43 100644 --- a/app/controllers/groups/group_links_controller.rb +++ b/app/controllers/groups/group_links_controller.rb @@ -24,7 +24,7 @@ class Groups::GroupLinksController < Groups::ApplicationController end def update - @group_link.update(group_link_params) + Groups::GroupLinks::UpdateService.new(@group_link).execute(group_link_params) end def destroy diff --git a/app/graphql/types/diff_refs_type.rb b/app/graphql/types/diff_refs_type.rb index 03d080d784b..4049a204f66 100644 --- a/app/graphql/types/diff_refs_type.rb +++ b/app/graphql/types/diff_refs_type.rb @@ -8,7 +8,7 @@ module Types field :head_sha, GraphQL::STRING_TYPE, null: false, description: 'SHA of the HEAD at the time the comment was made' - field :base_sha, GraphQL::STRING_TYPE, null: false, + field :base_sha, GraphQL::STRING_TYPE, null: true, description: 'Merge base of the branch the comment was made on' field :start_sha, GraphQL::STRING_TYPE, null: false, description: 'SHA of the branch being compared against' diff --git a/app/models/group.rb b/app/models/group.rb index ea5d46e23f4..bf771bd0409 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -509,18 +509,29 @@ class Group < Namespace group_group_links_query = GroupGroupLink.where(shared_group_id: self_and_ancestors_ids) cte = Gitlab::SQL::CTE.new(:group_group_links_cte, group_group_links_query) + cte_alias = cte.table.alias(GroupGroupLink.table_name) link = GroupGroupLink .with(cte.to_arel) + .select(smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]], + 'group_access')) .from([group_member_table, cte.alias_to(group_group_link_table)]) .where(group_member_table[:user_id].eq(user.id)) + .where(group_member_table[:requested_at].eq(nil)) .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id])) + .where(group_member_table[:source_type].eq('Namespace')) .reorder(Arel::Nodes::Descending.new(group_group_link_table[:group_access])) .first link&.group_access end + def smallest_value_arel(args, column_alias) + Arel::Nodes::As.new( + Arel::Nodes::NamedFunction.new('LEAST', args), + Arel::Nodes::SqlLiteral.new(column_alias)) + end + def self.groups_including_descendants_by(group_ids) Gitlab::ObjectHierarchy .new(Group.where(id: group_ids)) diff --git a/app/presenters/ci/pipeline_presenter.rb b/app/presenters/ci/pipeline_presenter.rb index 37abefb5664..ce9a3346b4b 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -134,7 +134,11 @@ module Ci def all_related_merge_requests strong_memoize(:all_related_merge_requests) do - pipeline.ref ? pipeline.all_merge_requests_by_recency.to_a : [] + if pipeline.ref && can?(current_user, :read_merge_request, pipeline.project) + pipeline.all_merge_requests_by_recency.to_a + else + [] + end end end end diff --git a/app/services/groups/group_links/destroy_service.rb b/app/services/groups/group_links/destroy_service.rb index 29aa8de4e68..6835b6c4637 100644 --- a/app/services/groups/group_links/destroy_service.rb +++ b/app/services/groups/group_links/destroy_service.rb @@ -6,19 +6,17 @@ module Groups def execute(one_or_more_links) links = Array(one_or_more_links) - GroupGroupLink.transaction do - GroupGroupLink.delete(links) + if GroupGroupLink.delete(links) + Gitlab::AppLogger.info( + "GroupGroupLinks with ids: #{links.map(&:id)} have been deleted.") groups_to_refresh = links.map(&:shared_with_group) groups_to_refresh.uniq.each do |group| group.refresh_members_authorized_projects end - - Gitlab::AppLogger.info("GroupGroupLinks with ids: #{links.map(&:id)} have been deleted.") - rescue => ex - Gitlab::AppLogger.error(ex) - - raise + else + Gitlab::AppLogger.info( + "Failed to delete GroupGroupLinks with ids: #{links.map(&:id)}.") end end end diff --git a/app/services/groups/group_links/update_service.rb b/app/services/groups/group_links/update_service.rb new file mode 100644 index 00000000000..71b52cb616c --- /dev/null +++ b/app/services/groups/group_links/update_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Groups + module GroupLinks + class UpdateService < BaseService + def initialize(group_link, user = nil) + super(group_link.shared_group, user) + + @group_link = group_link + end + + def execute(group_link_params) + group_link.update!(group_link_params) + + if requires_authorization_refresh?(group_link_params) + group_link.shared_with_group.refresh_members_authorized_projects + end + end + + private + + attr_accessor :group_link + + def requires_authorization_refresh?(params) + params.include?(:group_access) + end + end + end +end diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index bd70012c76c..52c73bcff03 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -16,17 +16,14 @@ module Projects @lfs_download_object = lfs_download_object end - # rubocop: disable CodeReuse/ActiveRecord def execute return unless project&.lfs_enabled? && lfs_download_object return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid? - return if LfsObject.exists?(oid: lfs_oid) wrap_download_errors do download_lfs_file! end end - # rubocop: enable CodeReuse/ActiveRecord private @@ -39,14 +36,24 @@ module Projects def download_lfs_file! with_tmp_file do |tmp_file| download_and_save_file!(tmp_file) - project.lfs_objects << LfsObject.new(oid: lfs_oid, - size: lfs_size, - file: tmp_file) + + project.lfs_objects << find_or_create_lfs_object(tmp_file) success end end + def find_or_create_lfs_object(tmp_file) + lfs_obj = LfsObject.safe_find_or_create_by!( + oid: lfs_oid, + size: lfs_size + ) + + lfs_obj.update!(file: tmp_file) unless lfs_obj.file.file + + lfs_obj + end + def download_and_save_file!(file) digester = Digest::SHA256.new response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment| diff --git a/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb b/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb index d6e6480bdad..75106297043 100644 --- a/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb +++ b/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb @@ -26,12 +26,12 @@ module Projects return [] end - # Getting all Lfs pointers already in the database and linking them to the project - linked_oids = LfsLinkService.new(project).execute(lfs_pointers_in_repository.keys) - # Retrieving those oids not present in the database which we need to download - missing_oids = lfs_pointers_in_repository.except(*linked_oids) - # Downloading the required information and gathering it inside a LfsDownloadObject for each oid - LfsDownloadLinkListService.new(project, remote_uri: current_endpoint_uri).execute(missing_oids) + # Downloading the required information and gathering it inside an + # LfsDownloadObject for each oid + # + LfsDownloadLinkListService + .new(project, remote_uri: current_endpoint_uri) + .execute(lfs_pointers_in_repository) rescue LfsDownloadLinkListService::DownloadLinksError => e raise LfsObjectDownloadListError, "The LFS objects download list couldn't be imported. Error: #{e.message}" end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 514ba998d2c..178a321e20c 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -13,8 +13,14 @@ class WebHookService end end + GITLAB_EVENT_HEADER = 'X-Gitlab-Event' + attr_accessor :hook, :data, :hook_name, :request_options + def self.hook_to_event(hook_name) + hook_name.to_s.singularize.titleize + end + def initialize(hook, data, hook_name) @hook = hook @data = data @@ -112,7 +118,7 @@ class WebHookService @headers ||= begin { 'Content-Type' => 'application/json', - 'X-Gitlab-Event' => hook_name.singularize.titleize + GITLAB_EVENT_HEADER => self.class.hook_to_event(hook_name) }.tap do |hash| hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present? end diff --git a/changelogs/unreleased/199035-sharing_group_to_update_project_authorization.yml b/changelogs/unreleased/199035-sharing_group_to_update_project_authorization.yml new file mode 100644 index 00000000000..00d0b770296 --- /dev/null +++ b/changelogs/unreleased/199035-sharing_group_to_update_project_authorization.yml @@ -0,0 +1,5 @@ +--- +title: Update ProjectAuthorization when deleting or updating GroupGroupLink +merge_request: +author: +type: security diff --git a/changelogs/unreleased/199415-sharing_group_to_respect_member_access_level.yml b/changelogs/unreleased/199415-sharing_group_to_respect_member_access_level.yml new file mode 100644 index 00000000000..bab1bf82dc0 --- /dev/null +++ b/changelogs/unreleased/199415-sharing_group_to_respect_member_access_level.yml @@ -0,0 +1,5 @@ +--- +title: Respect member access level for group shares +merge_request: +author: +type: security diff --git a/changelogs/unreleased/36805-confidential-issue.yml b/changelogs/unreleased/36805-confidential-issue.yml new file mode 100644 index 00000000000..ea7b66b89db --- /dev/null +++ b/changelogs/unreleased/36805-confidential-issue.yml @@ -0,0 +1,5 @@ +--- +title: Prevent an endless checking loop for two merge requests targeting each other +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-check-mr-permissions-for-pipeline-widget.yml b/changelogs/unreleased/security-check-mr-permissions-for-pipeline-widget.yml new file mode 100644 index 00000000000..009b205ee94 --- /dev/null +++ b/changelogs/unreleased/security-check-mr-permissions-for-pipeline-widget.yml @@ -0,0 +1,5 @@ +--- +title: Check merge requests read permissions before showing them in the pipeline widget +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-deprecate-lfs-link-service.yml b/changelogs/unreleased/security-deprecate-lfs-link-service.yml new file mode 100644 index 00000000000..79bc69414eb --- /dev/null +++ b/changelogs/unreleased/security-deprecate-lfs-link-service.yml @@ -0,0 +1,5 @@ +--- +title: Remove OID filtering during LFS imports +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-disable-pipeline-webhook-recursion.yml b/changelogs/unreleased/security-disable-pipeline-webhook-recursion.yml new file mode 100644 index 00000000000..a3491c1d42a --- /dev/null +++ b/changelogs/unreleased/security-disable-pipeline-webhook-recursion.yml @@ -0,0 +1,5 @@ +--- +title: Protect against denial of service using pipeline webhook recursion +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-graphql-diff-refs-empty-base-sha.yml b/changelogs/unreleased/security-graphql-diff-refs-empty-base-sha.yml new file mode 100644 index 00000000000..ba7906f72a8 --- /dev/null +++ b/changelogs/unreleased/security-graphql-diff-refs-empty-base-sha.yml @@ -0,0 +1,5 @@ +--- +title: Don't require base_sha in DiffRefsType +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-pb-fix-xss-dependency-link.yml b/changelogs/unreleased/security-pb-fix-xss-dependency-link.yml new file mode 100644 index 00000000000..a4726c3861a --- /dev/null +++ b/changelogs/unreleased/security-pb-fix-xss-dependency-link.yml @@ -0,0 +1,5 @@ +--- +title: Sanitize output by dependency linkers +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-recalculate_project_authorizations_run_2.yml b/changelogs/unreleased/security-recalculate_project_authorizations_run_2.yml new file mode 100644 index 00000000000..ee2039806b6 --- /dev/null +++ b/changelogs/unreleased/security-recalculate_project_authorizations_run_2.yml @@ -0,0 +1,5 @@ +--- +title: Recalculate ProjectAuthorizations for all users +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-safe-sentry-error-culprit.yml b/changelogs/unreleased/security-safe-sentry-error-culprit.yml new file mode 100644 index 00000000000..4261e2aa5dd --- /dev/null +++ b/changelogs/unreleased/security-safe-sentry-error-culprit.yml @@ -0,0 +1,5 @@ +--- +title: Escape special chars in Sentry error header +merge_request: +author: +type: security diff --git a/db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb b/db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb new file mode 100644 index 00000000000..8f4a347b5e2 --- /dev/null +++ b/db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class ScheduleRecalculateProjectAuthorizationsSecondRun < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + MIGRATION = 'RecalculateProjectAuthorizationsWithMinMaxUserId' + BATCH_SIZE = 2_500 + DELAY_INTERVAL = 2.minutes.to_i + + disable_ddl_transaction! + + class User < ActiveRecord::Base + include ::EachBatch + + self.table_name = 'users' + end + + def up + say "Scheduling #{MIGRATION} jobs" + + User.each_batch(of: BATCH_SIZE) do |batch, index| + delay = index * DELAY_INTERVAL + range = batch.pluck('MIN(id)', 'MAX(id)').first + BackgroundMigrationWorker.perform_in(delay, MIGRATION, range) + end + end + + def down + end +end diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index c5bf95e0aa6..605eba981aa 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -1530,7 +1530,7 @@ type DiffRefs { """ Merge base of the branch the comment was made on """ - baseSha: String! + baseSha: String """ SHA of the HEAD at the time the comment was made diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index b4724c73c54..615ae88077f 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -8105,13 +8105,9 @@ ], "type": { - "kind": "NON_NULL", - "name": null, - "ofType": { - "kind": "SCALAR", - "name": "String", - "ofType": null - } + "kind": "SCALAR", + "name": "String", + "ofType": null }, "isDeprecated": false, "deprecationReason": null diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 97b736d614d..e5e37339457 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -253,7 +253,7 @@ Autogenerated return type of DestroySnippet | Name | Type | Description | | --- | ---- | ---------- | -| `baseSha` | String! | Merge base of the branch the comment was made on | +| `baseSha` | String | Merge base of the branch the comment was made on | | `headSha` | String! | SHA of the HEAD at the time the comment was made | | `startSha` | String! | SHA of the branch being compared against | diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index ab83d84284f..76af29b2977 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -4,6 +4,8 @@ module API class Triggers < Grape::API include PaginationParams + HTTP_GITLAB_EVENT_HEADER = "HTTP_#{WebHookService::GITLAB_EVENT_HEADER}".underscore.upcase + params do requires :id, type: String, desc: 'The ID of a project' end @@ -19,6 +21,8 @@ module API post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42283') + forbidden! if gitlab_pipeline_hook_request? + # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } @@ -128,5 +132,11 @@ module API destroy_conditionally!(trigger) end end + + helpers do + def gitlab_pipeline_hook_request? + request.get_header(HTTP_GITLAB_EVENT_HEADER) == WebHookService.hook_to_event(:pipeline_hooks) + end + end end end diff --git a/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb b/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb new file mode 100644 index 00000000000..b66fdfd5c65 --- /dev/null +++ b/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # rubocop:disable Style/Documentation + class RecalculateProjectAuthorizationsWithMinMaxUserId + def perform(min_user_id, max_user_id) + User.where(id: min_user_id..max_user_id).find_each do |user| + service = Users::RefreshAuthorizedProjectsService.new( + user, + incorrect_auth_found_callback: + ->(project_id, access_level) do + logger.info(message: 'Removing ProjectAuthorizations', + user_id: user.id, + project_id: project_id, + access_level: access_level) + end, + missing_auth_found_callback: + ->(project_id, access_level) do + logger.info(message: 'Creating ProjectAuthorizations', + user_id: user.id, + project_id: project_id, + access_level: access_level) + end + ) + + service.execute + end + end + + private + + def logger + @logger ||= Gitlab::BackgroundMigration::Logger.build + end + end + end +end diff --git a/lib/gitlab/dependency_linker/base_linker.rb b/lib/gitlab/dependency_linker/base_linker.rb index dd7ab92c6ae..a4e265eba88 100644 --- a/lib/gitlab/dependency_linker/base_linker.rb +++ b/lib/gitlab/dependency_linker/base_linker.rb @@ -7,6 +7,8 @@ module Gitlab GIT_INVALID_URL_REGEX = /^git\+#{URL_REGEX}/.freeze REPO_REGEX = %r{[^/'" ]+/[^/'" ]+}.freeze + include ActionView::Helpers::SanitizeHelper + class_attribute :file_type def self.support?(blob_name) @@ -62,7 +64,10 @@ module Gitlab end def link_tag(name, url) - %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>}.html_safe + sanitize( + %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>}, + attributes: %w[href rel target] + ) end # Links package names based on regex. diff --git a/lib/gitlab/project_authorizations.rb b/lib/gitlab/project_authorizations.rb index d65e8759ec9..ff90a009b2e 100644 --- a/lib/gitlab/project_authorizations.rb +++ b/lib/gitlab/project_authorizations.rb @@ -62,6 +62,7 @@ module Gitlab cte = Gitlab::SQL::RecursiveCTE.new(:namespaces_cte) members = Member.arel_table namespaces = Namespace.arel_table + group_group_links = GroupGroupLink.arel_table # Namespaces the user is a member of. cte << user.groups @@ -69,7 +70,10 @@ module Gitlab .except(:order) # Namespaces shared with any of the group - cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level']) + cte << Group.select([namespaces[:id], + least(members[:access_level], + group_group_links[:group_access], + 'access_level')]) .joins(join_group_group_links) .joins(join_members_on_group_group_links) diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 097b502316e..a00e72f7aad 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -67,7 +67,13 @@ module Gitlab return false unless can_access_git? return false unless project - return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref) + # Checking for an internal project to prevent an infinite loop: + # https://gitlab.com/gitlab-org/gitlab/issues/36805 + if project.internal? + return false unless user.can?(:push_code, project) + else + return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref) + end if protected?(ProtectedBranch, project, ref) protected_branch_accessible_to?(ref, action: :push) diff --git a/spec/controllers/groups/group_links_controller_spec.rb b/spec/controllers/groups/group_links_controller_spec.rb index c062de468fc..21169188386 100644 --- a/spec/controllers/groups/group_links_controller_spec.rb +++ b/spec/controllers/groups/group_links_controller_spec.rb @@ -6,9 +6,13 @@ describe Groups::GroupLinksController do let(:shared_with_group) { create(:group, :private) } let(:shared_group) { create(:group, :private) } let(:user) { create(:user) } + let(:group_member) { create(:user) } + let!(:project) { create(:project, group: shared_group) } before do sign_in(user) + + shared_with_group.add_developer(group_member) end describe '#create' do @@ -40,13 +44,9 @@ describe Groups::GroupLinksController do end context 'when user has correct access to both groups' do - let(:group_member) { create(:user) } - before do shared_with_group.add_developer(user) shared_group.add_owner(user) - - shared_with_group.add_developer(group_member) end context 'when default access level is requested' do @@ -56,6 +56,10 @@ describe Groups::GroupLinksController do context 'when owner access is requested' do let(:shared_group_access) { Gitlab::Access::OWNER } + before do + shared_with_group.add_owner(group_member) + end + include_examples 'creates group group link' it 'allows admin access for group member' do @@ -64,6 +68,10 @@ describe Groups::GroupLinksController do end end + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:read_project, project) }.from(false).to(true) + end + context 'when shared with group id is not present' do let(:shared_with_group_id) { nil } @@ -149,6 +157,7 @@ describe Groups::GroupLinksController do context 'when user has admin access to the shared group' do before do shared_group.add_owner(user) + shared_with_group.refresh_members_authorized_projects end it 'updates existing link' do @@ -162,6 +171,10 @@ describe Groups::GroupLinksController do expect(link.group_access).to eq(Gitlab::Access::GUEST) expect(link.expires_at).to eq(expiry_date) end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end end context 'when user does not have admin access to the shared group' do @@ -199,11 +212,16 @@ describe Groups::GroupLinksController do context 'when user has admin access to the shared group' do before do shared_group.add_owner(user) + shared_with_group.refresh_members_authorized_projects end it 'deletes existing link' do expect { subject }.to change(GroupGroupLink, :count).by(-1) end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end end context 'when user does not have admin access to the shared group' do diff --git a/spec/frontend/error_tracking/components/error_details_spec.js b/spec/frontend/error_tracking/components/error_details_spec.js index 94bf0189c91..ca3431920fe 100644 --- a/spec/frontend/error_tracking/components/error_details_spec.js +++ b/spec/frontend/error_tracking/components/error_details_spec.js @@ -130,6 +130,28 @@ describe('ErrorDetails', () => { expect(wrapper.findAll('button').length).toBe(3); }); + describe('unsafe chars for culprit field', () => { + const findReportedText = () => wrapper.find('[data-qa-selector="reported_text"]'); + const culprit = '<script>console.log("surprise!")</script>'; + beforeEach(() => { + store.state.details.loadingStacktrace = false; + wrapper.setData({ + error: { + culprit, + }, + }); + }); + + it('should not convert interpolated text to html entities', () => { + expect(findReportedText().findAll('script').length).toEqual(0); + expect(findReportedText().findAll('strong').length).toEqual(1); + }); + + it('should render text instead of converting to html entities', () => { + expect(findReportedText().text()).toContain(culprit); + }); + }); + describe('Badges', () => { it('should show language and error level badges', () => { wrapper.setData({ diff --git a/spec/graphql/types/diff_refs_type_spec.rb b/spec/graphql/types/diff_refs_type_spec.rb index 91017c827ad..85225e5809c 100644 --- a/spec/graphql/types/diff_refs_type_spec.rb +++ b/spec/graphql/types/diff_refs_type_spec.rb @@ -5,5 +5,9 @@ require 'spec_helper' describe GitlabSchema.types['DiffRefs'] do it { expect(described_class.graphql_name).to eq('DiffRefs') } - it { expect(described_class).to have_graphql_fields(:base_sha, :head_sha, :start_sha) } + it { is_expected.to have_graphql_fields(:head_sha, :base_sha, :start_sha).only } + + it { expect(described_class.fields['headSha'].type).to be_non_null } + it { expect(described_class.fields['baseSha'].type).not_to be_non_null } + it { expect(described_class.fields['startSha'].type).to be_non_null } end diff --git a/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb b/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb new file mode 100644 index 00000000000..14ba57eecbf --- /dev/null +++ b/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::RecalculateProjectAuthorizationsWithMinMaxUserId, :migration, schema: 20200204113224 do + let(:users_table) { table(:users) } + let(:min) { 1 } + let(:max) { 5 } + + before do + min.upto(max) do |i| + users_table.create!(id: i, email: "user#{i}@example.com", projects_limit: 10) + end + end + + describe '#perform' do + it 'initializes Users::RefreshAuthorizedProjectsService with correct users' do + min.upto(max) do |i| + user = User.find(i) + expect(Users::RefreshAuthorizedProjectsService).to( + receive(:new).with(user, any_args).and_call_original) + end + + described_class.new.perform(min, max) + end + + it 'executes Users::RefreshAuthorizedProjectsService' do + expected_call_counts = max - min + 1 + + service = instance_double(Users::RefreshAuthorizedProjectsService) + expect(Users::RefreshAuthorizedProjectsService).to( + receive(:new).exactly(expected_call_counts).times.and_return(service)) + expect(service).to receive(:execute).exactly(expected_call_counts).times + + described_class.new.perform(min, max) + end + end +end diff --git a/spec/lib/gitlab/dependency_linker/base_linker_spec.rb b/spec/lib/gitlab/dependency_linker/base_linker_spec.rb new file mode 100644 index 00000000000..1466ce2dfcc --- /dev/null +++ b/spec/lib/gitlab/dependency_linker/base_linker_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::DependencyLinker::BaseLinker do + let(:linker_class) do + Class.new(described_class) do + def link_dependencies + link_regex(%r{^(?<name>https?://[^ ]+)}, &:itself) + end + end + end + + let(:plain_content) do + <<~CONTENT + http://\\njavascript:alert(1) + https://gitlab.com/gitlab-org/gitlab + CONTENT + end + + let(:highlighted_content) do + <<~CONTENT + <span><span>http://</span><span>\\n</span><span>javascript:alert(1)</span></span> + <span><span>https://gitlab.com/gitlab-org/gitlab</span></span> + CONTENT + end + + let(:linker) { linker_class.new(plain_content, highlighted_content) } + + describe '#link' do + subject { linker.link } + + it 'only converts valid links' do + expect(subject).to eq( + <<~CONTENT + <span><span>#{link('http://')}</span><span>#{link('\n', url: '%5Cn')}</span><span>#{link('javascript:alert(1)', url: nil)}</span></span> + <span><span>#{link('https://gitlab.com/gitlab-org/gitlab')}</span></span> + CONTENT + ) + end + end + + def link(text, url: text) + attrs = [ + 'rel="nofollow noreferrer noopener"', + 'target="_blank"' + ] + + attrs.unshift(%{href="#{url}"}) if url + + %{<a #{attrs.join(' ')}>#{text}</a>} + end +end diff --git a/spec/lib/gitlab/project_authorizations_spec.rb b/spec/lib/gitlab/project_authorizations_spec.rb index 1c579128223..7b282433061 100644 --- a/spec/lib/gitlab/project_authorizations_spec.rb +++ b/spec/lib/gitlab/project_authorizations_spec.rb @@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do end end + context 'with lower group access level than max access level for share' do + let(:user) { create(:user) } + + it 'creates proper authorizations' do + group.add_reporter(user) + + mapping = map_access_levels(authorizations) + + expect(mapping[project_parent.id]).to be_nil + expect(mapping[project.id]).to eq(Gitlab::Access::REPORTER) + expect(mapping[project_child.id]).to eq(Gitlab::Access::REPORTER) + end + end + context 'parent group user' do let(:user) { parent_group_user } diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index 2f4ab2e71db..181ea1e7fd3 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -30,6 +30,17 @@ describe Gitlab::UserAccess do end end + describe 'push to branch in an internal project' do + it 'will not infinitely loop when a project is internal' do + project.visibility_level = Gitlab::VisibilityLevel::INTERNAL + project.save! + + expect(project).not_to receive(:branch_allows_collaboration?) + + access.can_push_to_branch?('master') + end + end + describe 'push to empty project' do let(:empty_project) { create(:project_empty_repo) } let(:project_access) { described_class.new(user, project: empty_project) } diff --git a/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb b/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb new file mode 100644 index 00000000000..04726f98c89 --- /dev/null +++ b/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20200204113224_schedule_recalculate_project_authorizations_second_run.rb') + +describe ScheduleRecalculateProjectAuthorizationsSecondRun, :migration do + let(:users_table) { table(:users) } + + before do + stub_const("#{described_class}::BATCH_SIZE", 2) + + 1.upto(4) do |i| + users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1) + end + end + + it 'schedules background migration' do + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + expect(described_class::MIGRATION).to be_scheduled_migration(1, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(3, 4) + end + end + end +end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 3531c695236..d42888e1d54 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -563,6 +563,18 @@ describe Group do expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) end + + context 'with lower group access level than max access level for share' do + let(:user) { create(:user) } + + it 'returns correct access level' do + group.add_reporter(user) + + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + end + end end context 'with user in the parent group' do @@ -584,6 +596,33 @@ describe Group do expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) end end + + context 'unrelated project owner' do + let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 } + let!(:group) { create(:group, id: common_id) } + let!(:unrelated_project) { create(:project, id: common_id) } + let(:user) { unrelated_project.owner } + + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'user without accepted access request' do + let!(:user) { create(:user) } + + before do + create(:group_member, :developer, :access_request, user: user, group: group) + end + + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + end + end end context 'when feature flag share_group_with_group is disabled' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6c90a1b5614..50409615cd0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4793,6 +4793,38 @@ describe Project do end end + context 'with cross internal project merge requests' do + let(:project) { create(:project, :repository, :internal) } + let(:forked_project) { fork_project(project, nil, repository: true) } + let(:user) { double(:user) } + + it "does not endlessly loop for internal projects with MRs to each other", :sidekiq_inline do + allow(user).to receive(:can?).and_return(true, false, true) + allow(user).to receive(:id).and_return(1) + + create( + :merge_request, + target_project: project, + target_branch: 'merge-test', + source_project: forked_project, + source_branch: 'merge-test', + allow_collaboration: true + ) + + create( + :merge_request, + target_project: forked_project, + target_branch: 'merge-test', + source_project: project, + source_branch: 'merge-test', + allow_collaboration: true + ) + + expect(user).to receive(:can?).at_most(5).times + project.branch_allows_collaboration?(user, "merge-test") + end + end + context 'with cross project merge requests' do let(:user) { create(:user) } let(:target_project) { create(:project, :repository) } diff --git a/spec/presenters/ci/pipeline_presenter_spec.rb b/spec/presenters/ci/pipeline_presenter_spec.rb index 700d1f5cbb6..c9c4f567549 100644 --- a/spec/presenters/ci/pipeline_presenter_spec.rb +++ b/spec/presenters/ci/pipeline_presenter_spec.rb @@ -6,6 +6,7 @@ describe Ci::PipelinePresenter do include Gitlab::Routing let(:user) { create(:user) } + let(:current_user) { user } let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } @@ -15,7 +16,7 @@ describe Ci::PipelinePresenter do before do project.add_developer(user) - allow(presenter).to receive(:current_user) { user } + allow(presenter).to receive(:current_user) { current_user } end it 'inherits from Gitlab::View::Presenter::Delegated' do @@ -224,10 +225,90 @@ describe Ci::PipelinePresenter do describe '#all_related_merge_requests' do it 'memoizes the returned relation' do query_count = ActiveRecord::QueryRecorder.new do - 2.times { presenter.send(:all_related_merge_requests).count } + 3.times { presenter.send(:all_related_merge_requests).count } end.count - expect(query_count).to eq(1) + expect(query_count).to eq(2) + end + + context 'permissions' do + let!(:merge_request) do + create(:merge_request, project: project, source_project: project) + end + + subject(:all_related_merge_requests) do + presenter.send(:all_related_merge_requests) + end + + shared_examples 'private merge requests' do + context 'when not logged in' do + let(:current_user) {} + + it { is_expected.to be_empty } + end + + context 'when logged in as a non_member' do + let(:current_user) { create(:user) } + + it { is_expected.to be_empty } + end + + context 'when logged in as a guest' do + let(:current_user) { create(:user) } + + before do + project.add_guest(current_user) + end + + it { is_expected.to be_empty } + end + + context 'when logged in as a developer' do + it { is_expected.to contain_exactly(merge_request) } + end + + context 'when logged in as a maintainer' do + let(:current_user) { create(:user) } + + before do + project.add_maintainer(current_user) + end + + it { is_expected.to contain_exactly(merge_request) } + end + end + + context 'with a private project' do + it_behaves_like 'private merge requests' + end + + context 'with a public project with private merge requests' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + project + .project_feature + .update!(merge_requests_access_level: ProjectFeature::PRIVATE) + end + + it_behaves_like 'private merge requests' + end + + context 'with a public project with public merge requests' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + project + .project_feature + .update!(merge_requests_access_level: ProjectFeature::ENABLED) + end + + context 'when not logged in' do + let(:current_user) {} + + it { is_expected.to contain_exactly(merge_request) } + end + end end end diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 35e41f5ae52..1042e4e970d 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -116,6 +116,18 @@ describe API::Triggers do end end end + + context 'when is triggered by a pipeline hook' do + it 'does not create a new pipeline' do + expect do + post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"), + params: { ref: 'refs/heads/other-branch' }, + headers: { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' } + end.not_to change(Ci::Pipeline, :count) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end describe 'GET /projects/:id/triggers' do diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index 6f49b6eda94..284bcd0df2e 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -40,24 +40,11 @@ describe Groups::GroupLinks::DestroyService, '#execute' do end it 'updates project authorization once per group' do - expect(GroupGroupLink).to receive(:delete) + expect(GroupGroupLink).to receive(:delete).and_call_original expect(group).to receive(:refresh_members_authorized_projects).once expect(another_group).to receive(:refresh_members_authorized_projects).once subject.execute(links) end - - it 'rolls back changes when error happens' do - group.add_developer(user) - - expect(group).to receive(:refresh_members_authorized_projects).once.and_call_original - expect(another_group).to( - receive(:refresh_members_authorized_projects).and_raise('boom')) - - expect { subject.execute(links) }.to raise_error('boom') - - expect(GroupGroupLink.count).to eq(links.length) - expect(Ability.allowed?(user, :read_project, project)).to be_truthy - end end end diff --git a/spec/services/groups/group_links/update_service_spec.rb b/spec/services/groups/group_links/update_service_spec.rb new file mode 100644 index 00000000000..446364c9799 --- /dev/null +++ b/spec/services/groups/group_links/update_service_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Groups::GroupLinks::UpdateService, '#execute' do + let(:user) { create(:user) } + + let_it_be(:group) { create(:group, :private) } + let_it_be(:shared_group) { create(:group, :private) } + let_it_be(:project) { create(:project, group: shared_group) } + let(:group_member) { create(:user) } + let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) } + + let(:expiry_date) { 1.month.from_now.to_date } + let(:group_link_params) do + { group_access: Gitlab::Access::GUEST, + expires_at: expiry_date } + end + + subject { described_class.new(link).execute(group_link_params) } + + before do + group.add_developer(group_member) + end + + it 'updates existing link' do + expect(link.group_access).to eq(Gitlab::Access::DEVELOPER) + expect(link.expires_at).to be_nil + + subject + + link.reload + + expect(link.group_access).to eq(Gitlab::Access::GUEST) + expect(link.expires_at).to eq(expiry_date) + end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end + + it 'executes UserProjectAccessChangedService' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute) + end + + subject + end + + context 'with only param not requiring authorization refresh' do + let(:group_link_params) { { expires_at: Date.tomorrow } } + + it 'does not execute UserProjectAccessChangedService' do + expect(UserProjectAccessChangedService).not_to receive(:new) + + subject + end + end +end diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 21a139cdf3c..496d1fe67f2 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -134,6 +134,21 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when an lfs object with the same oid already exists' do + let!(:existing_lfs_object) { create(:lfs_object, oid: oid) } + + before do + stub_full_request(download_link).to_return(body: lfs_content) + end + + it_behaves_like 'no lfs object is created' + + it 'does not update the file attached to the existing LfsObject' do + expect { subject.execute } + .not_to change { existing_lfs_object.reload.file.file.file } + end + end + context 'when credentials present' do let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" } let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) } @@ -211,17 +226,5 @@ describe Projects::LfsPointers::LfsDownloadService do subject.execute end end - - context 'when an lfs object with the same oid already exists' do - before do - create(:lfs_object, oid: oid) - end - - it 'does not download the file' do - expect(subject).not_to receive(:download_lfs_file!) - - subject.execute - end - end end end diff --git a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb index 9dac29765a2..e94d8a85987 100644 --- a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb @@ -24,7 +24,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do describe '#execute' do context 'when no lfs pointer is linked' do before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([]) allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links) expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original end @@ -35,12 +34,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do subject.execute end - it 'links existent lfs objects to the project' do - expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute) - - subject.execute - end - it 'retrieves the download links of non existent objects' do expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids) @@ -48,32 +41,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do end end - context 'when some lfs objects are linked' do - before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys) - allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links) - end - - it 'retrieves the download links of non existent objects' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids) - - subject.execute - end - end - - context 'when all lfs objects are linked' do - before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys) - allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute) - end - - it 'retrieves no download links' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original - - expect(subject.execute).to be_empty - end - end - context 'when lfsconfig file exists' do before do allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n") |