diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-02-28 20:09:26 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-02-28 20:09:26 +0000 |
commit | 952784c9a19731aad0bdccba5bfc7d3bbce850e3 (patch) | |
tree | 678090f5f2b33ed26859ddb839ee5a5ca5ae3b2e | |
parent | c0b4e483c6ef80cf5c9c02abf74d2eb7954b3622 (diff) | |
download | gitlab-ce-952784c9a19731aad0bdccba5bfc7d3bbce850e3.tar.gz |
Add latest changes from gitlab-org/security/gitlab@12-7-stable-ee
40 files changed, 539 insertions, 117 deletions
diff --git a/app/assets/javascripts/error_tracking/components/error_details.vue b/app/assets/javascripts/error_tracking/components/error_details.vue index 819d501cba6..240a5a26bcd 100644 --- a/app/assets/javascripts/error_tracking/components/error_details.vue +++ b/app/assets/javascripts/error_tracking/components/error_details.vue @@ -2,7 +2,7 @@ import { mapActions, mapGetters, mapState } from 'vuex'; import dateFormat from 'dateformat'; import createFlash from '~/flash'; -import { GlButton, GlFormInput, GlLink, GlLoadingIcon, GlBadge } from '@gitlab/ui'; +import { GlButton, GlFormInput, GlLink, GlLoadingIcon, GlBadge, GlSprintf } from '@gitlab/ui'; import { __, sprintf, n__ } from '~/locale'; import LoadingButton from '~/vue_shared/components/loading_button.vue'; import Icon from '~/vue_shared/components/icon.vue'; @@ -25,6 +25,7 @@ export default { Icon, Stacktrace, GlBadge, + GlSprintf, }, directives: { TrackEvent: TrackEventDirective, @@ -99,16 +100,6 @@ export default { 'updatingIgnoreStatus', ]), ...mapGetters('details', ['stacktrace']), - reported() { - return sprintf( - __('Reported %{timeAgo} by %{reportedBy}'), - { - reportedBy: `<strong>${this.GQLerror.culprit}</strong>`, - timeAgo: this.timeFormatted(this.stacktraceData.date_received), - }, - false, - ); - }, firstReleaseLink() { return `${this.error.external_base_url}/releases/${this.GQLerror.firstReleaseShortVersion}`; }, @@ -176,7 +167,17 @@ export default { </div> <div v-else-if="showDetails" class="error-details"> <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>{{ GQLerror.culprit }}</strong> + </template> + <template #timeAgo> + {{ timeFormatted(stacktraceData.date_received) }} + </template> + </gl-sprintf> + </div> + <div class="d-inline-flex"> <loading-button :label="__('Ignore')" 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 b642b177df1..a8d40734a7d 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -505,18 +505,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 f01ff56540a..27aa85c6836 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -128,7 +128,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 a009f479d5d..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.all_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 87edac36e33..1115612eac1 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -11,8 +11,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 @@ -110,7 +116,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-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/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 0884f8948f0..f3f74cf08ad 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -1281,7 +1281,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 b1617edbfbb..2d396cc053d 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -7247,13 +7247,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 5adc68209b0..829ee42b3fe 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -221,7 +221,7 @@ Autogenerated return type of DestroySnippet | Name | Type | Description | | --- | ---- | ---------- | | `headSha` | String! | SHA of the HEAD at the time the comment was made | -| `baseSha` | String! | Merge base of the branch the comment was made on | +| `baseSha` | String | Merge base of the branch the comment was made on | | `startSha` | String! | SHA of the branch being compared against | ## Discussion 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/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 04f2e33b26a..2f5170f3a2e 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 @@ -22,13 +26,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 it 'links group with selected group' do @@ -45,6 +45,10 @@ describe Groups::GroupLinksController do expect { subject }.to change { group_member.can?(:read_group, shared_group) }.from(false).to(true) 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 } @@ -130,6 +134,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 @@ -143,6 +148,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 @@ -180,11 +189,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 35014b00dd8..a43ee145960 100644 --- a/spec/frontend/error_tracking/components/error_details_spec.js +++ b/spec/frontend/error_tracking/components/error_details_spec.js @@ -1,6 +1,6 @@ import { createLocalVue, shallowMount } from '@vue/test-utils'; import Vuex from 'vuex'; -import { GlLoadingIcon, GlLink, GlBadge, GlFormInput } from '@gitlab/ui'; +import { GlLoadingIcon, GlLink, GlBadge, GlFormInput, GlSprintf } from '@gitlab/ui'; import LoadingButton from '~/vue_shared/components/loading_button.vue'; import Stacktrace from '~/error_tracking/components/stacktrace.vue'; import ErrorDetails from '~/error_tracking/components/error_details.vue'; @@ -22,7 +22,7 @@ describe('ErrorDetails', () => { function mountComponent() { wrapper = shallowMount(ErrorDetails, { - stubs: { LoadingButton }, + stubs: { LoadingButton, GlSprintf }, localVue, store, mocks, @@ -128,6 +128,30 @@ 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; + store.state.details.loading = false; + mountComponent(); + wrapper.setData({ + GQLerror: { + 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', () => { store.state.details.error.tags = { level: 'error', logger: 'ruby' }; 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/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 4e7c43a6856..460bc58c332 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/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 c57f47b5738..eb3fe72a744 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4795,6 +4795,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 fd391478eb4..533926aa6ec 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 @@ -215,10 +216,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 d54d112cd9f..1c415ffebee 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 970e82e7107..d73b6488062 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -133,6 +133,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) } @@ -210,17 +225,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") |