diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-12-04 16:53:29 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-12-04 16:53:29 +0000 |
commit | da2732bd2c56b6f6a7d8cd4e068976e03bb0350e (patch) | |
tree | 73fcee908d8b75b20e1a4154663c900f99350150 | |
parent | ec4a1458f6333332cbc345f9de57bdf15d16667a (diff) | |
download | gitlab-ce-da2732bd2c56b6f6a7d8cd4e068976e03bb0350e.tar.gz |
Add latest changes from gitlab-org/security/gitlab@13-5-stable-ee
26 files changed, 462 insertions, 39 deletions
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 672f36dedc0..05573255066 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -19,7 +19,7 @@ class UsersController < ApplicationController prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } before_action :user, except: [:exists, :suggests] before_action :authorize_read_user_profile!, - only: [:calendar, :calendar_activities, :groups, :projects, :contributed_projects, :starred_projects, :snippets] + only: [:calendar, :calendar_activities, :groups, :projects, :contributed, :starred, :snippets] feature_category :users diff --git a/app/finders/starred_projects_finder.rb b/app/finders/starred_projects_finder.rb index fcb469d1d17..e209960c471 100644 --- a/app/finders/starred_projects_finder.rb +++ b/app/finders/starred_projects_finder.rb @@ -1,11 +1,22 @@ # frozen_string_literal: true class StarredProjectsFinder < ProjectsFinder + include Gitlab::Allowable + def initialize(user, params: {}, current_user: nil) + @user = user + super( params: params, current_user: current_user, project_ids_relation: user.starred_projects.select(:id) ) end + + def execute + # Do not show starred projects if the user has a private profile. + return Project.none unless can?(current_user, :read_user_profile, @user) + + super + end end diff --git a/app/graphql/types/user_type.rb b/app/graphql/types/user_type.rb index efd78e17958..4fa62c49611 100644 --- a/app/graphql/types/user_type.rb +++ b/app/graphql/types/user_type.rb @@ -30,13 +30,11 @@ module Types resolver: Resolvers::TodoResolver, description: 'Todos of the user' field :group_memberships, Types::GroupMemberType.connection_type, null: true, - description: 'Group memberships of the user', - method: :group_members + description: 'Group memberships of the user' field :status, Types::UserStatusType, null: true, description: 'User status' field :project_memberships, Types::ProjectMemberType.connection_type, null: true, - description: 'Project memberships of the user', - method: :project_members + description: 'Project memberships of the user' field :starred_projects, Types::ProjectType.connection_type, null: true, description: 'Projects starred by the user', resolver: Resolvers::UserStarredProjectsResolver diff --git a/app/presenters/user_presenter.rb b/app/presenters/user_presenter.rb index f201b36346f..0028e6d9ef0 100644 --- a/app/presenters/user_presenter.rb +++ b/app/presenters/user_presenter.rb @@ -2,4 +2,18 @@ class UserPresenter < Gitlab::View::Presenter::Delegated presents :user + + def group_memberships + should_be_private? ? GroupMember.none : user.group_members + end + + def project_memberships + should_be_private? ? ProjectMember.none : user.project_members + end + + private + + def should_be_private? + !can?(current_user, :read_user_profile, user) + end end diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb index 97c56b84434..7cfedc2233a 100644 --- a/app/services/todos/destroy/entity_leave_service.rb +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -22,7 +22,7 @@ module Todos # if at least reporter, all entities including confidential issues can be accessed return if user_has_reporter_access? - remove_confidential_issue_todos + remove_confidential_resource_todos if entity.private? remove_project_todos @@ -40,7 +40,7 @@ module Todos end end - def remove_confidential_issue_todos + def remove_confidential_resource_todos Todo .for_target(confidential_issues.select(:id)) .for_type(Issue.name) @@ -133,3 +133,5 @@ module Todos end end end + +Todos::Destroy::EntityLeaveService.prepend_if_ee('EE::Todos::Destroy::EntityLeaveService') diff --git a/app/validators/zoom_url_validator.rb b/app/validators/zoom_url_validator.rb index dc4ca6b9501..e0f8e4e34a2 100644 --- a/app/validators/zoom_url_validator.rb +++ b/app/validators/zoom_url_validator.rb @@ -5,8 +5,13 @@ # Custom validator for zoom urls # class ZoomUrlValidator < ActiveModel::EachValidator + ALLOWED_SCHEMES = %w(https).freeze + def validate_each(record, attribute, value) - return if Gitlab::ZoomLinkExtractor.new(value).links.size == 1 + links_count = Gitlab::ZoomLinkExtractor.new(value).links.size + valid = Gitlab::UrlSanitizer.valid?(value, allowed_schemes: ALLOWED_SCHEMES) + + return if links_count == 1 && valid record.errors.add(:url, 'must contain one valid Zoom URL') end diff --git a/app/views/devise/confirmations/new.html.haml b/app/views/devise/confirmations/new.html.haml index f8aa3cf98dc..8c0d076c77d 100644 --- a/app/views/devise/confirmations/new.html.haml +++ b/app/views/devise/confirmations/new.html.haml @@ -6,7 +6,7 @@ = render "devise/shared/error_messages", resource: resource .form-group = f.label :email - = f.email_field :email, class: "form-control", required: true, title: 'Please provide a valid email address.' + = f.email_field :email, class: "form-control", required: true, title: 'Please provide a valid email address.', value: nil .clearfix = f.submit "Resend", class: 'btn btn-success' diff --git a/changelogs/unreleased/security-296-private_profile_exposure.yml b/changelogs/unreleased/security-296-private_profile_exposure.yml new file mode 100644 index 00000000000..05d98788aed --- /dev/null +++ b/changelogs/unreleased/security-296-private_profile_exposure.yml @@ -0,0 +1,5 @@ +--- +title: Ensure group and project memberships are not leaked via API for users with private profiles +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-hide-email-in-confirmation-page.yml b/changelogs/unreleased/security-hide-email-in-confirmation-page.yml new file mode 100644 index 00000000000..b8f448acfcd --- /dev/null +++ b/changelogs/unreleased/security-hide-email-in-confirmation-page.yml @@ -0,0 +1,5 @@ +--- +title: Do not show emails of users in confirmation page +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-project-import-zoom-xss.yml b/changelogs/unreleased/security-project-import-zoom-xss.yml new file mode 100644 index 00000000000..4f4d7f14b6b --- /dev/null +++ b/changelogs/unreleased/security-project-import-zoom-xss.yml @@ -0,0 +1,5 @@ +--- +title: Validate zoom links to start with https only +merge_request: 1055 +author: +type: security diff --git a/changelogs/unreleased/security-starred-projects-api-fix.yml b/changelogs/unreleased/security-starred-projects-api-fix.yml new file mode 100644 index 00000000000..efb12998393 --- /dev/null +++ b/changelogs/unreleased/security-starred-projects-api-fix.yml @@ -0,0 +1,5 @@ +--- +title: Do not expose starred projects of users with private profile via API +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-starred-projects-private-profile.yml b/changelogs/unreleased/security-starred-projects-private-profile.yml new file mode 100644 index 00000000000..1fb47dce518 --- /dev/null +++ b/changelogs/unreleased/security-starred-projects-private-profile.yml @@ -0,0 +1,5 @@ +--- +title: Do not show starred & contributed projects of users with private profile +merge_request: +author: +type: security diff --git a/config/feature_categories.yml b/config/feature_categories.yml index edf7bba27a3..f8e44f56a9e 100644 --- a/config/feature_categories.yml +++ b/config/feature_categories.yml @@ -46,6 +46,7 @@ - dynamic_application_security_testing - editor_extension - epics +- epic_tracking - error_tracking - feature_flags - foundations diff --git a/db/post_migrate/20201109114603_schedule_remove_inaccessible_epic_todos.rb b/db/post_migrate/20201109114603_schedule_remove_inaccessible_epic_todos.rb new file mode 100644 index 00000000000..13d12675a28 --- /dev/null +++ b/db/post_migrate/20201109114603_schedule_remove_inaccessible_epic_todos.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class ScheduleRemoveInaccessibleEpicTodos < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INTERVAL = 2.minutes + BATCH_SIZE = 10 + MIGRATION = 'RemoveInaccessibleEpicTodos' + + disable_ddl_transaction! + + class Epic < ActiveRecord::Base + include EachBatch + end + + def up + return unless Gitlab.ee? + + relation = Epic.where(confidential: true) + + queue_background_migration_jobs_by_range_at_intervals( + relation, MIGRATION, INTERVAL, batch_size: BATCH_SIZE) + end + + def down + # no-op + end +end diff --git a/db/schema_migrations/20201109114603 b/db/schema_migrations/20201109114603 new file mode 100644 index 00000000000..c1df2223dbd --- /dev/null +++ b/db/schema_migrations/20201109114603 @@ -0,0 +1 @@ +ae8034ec52df47ce2ce3397715dd18347e4d297a963c17c7b26321f414dfa632
\ No newline at end of file diff --git a/doc/user/todos.md b/doc/user/todos.md index c48d2adfa45..9d123c48d2e 100644 --- a/doc/user/todos.md +++ b/doc/user/todos.md @@ -64,7 +64,7 @@ To do item triggers aren't affected by [GitLab notification email settings](prof NOTE: **Note:** When a user no longer has access to a resource related to a to do item (such as -an issue, merge request, project, or group), for security reasons GitLab +an issue, merge request, epic, project, or group), for security reasons GitLab deletes any related to do items within the next hour. Deletion is delayed to prevent data loss, in the case where a user's access is accidentally revoked. diff --git a/lib/gitlab/background_migration/remove_inaccessible_epic_todos.rb b/lib/gitlab/background_migration/remove_inaccessible_epic_todos.rb new file mode 100644 index 00000000000..74c48b237cc --- /dev/null +++ b/lib/gitlab/background_migration/remove_inaccessible_epic_todos.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # rubocop:disable Style/Documentation + class RemoveInaccessibleEpicTodos + def perform(start_id, stop_id) + end + end + end +end + +Gitlab::BackgroundMigration::RemoveInaccessibleEpicTodos.prepend_if_ee('EE::Gitlab::BackgroundMigration::RemoveInaccessibleEpicTodos') diff --git a/spec/controllers/confirmations_controller_spec.rb b/spec/controllers/confirmations_controller_spec.rb new file mode 100644 index 00000000000..49a39f257fe --- /dev/null +++ b/spec/controllers/confirmations_controller_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ConfirmationsController do + include DeviseHelpers + + before do + set_devise_mapping(context: @request) + end + + describe '#show' do + render_views + + subject { get :show, params: { confirmation_token: confirmation_token } } + + context 'user is already confirmed' do + let_it_be_with_reload(:user) { create(:user, :unconfirmed) } + let(:confirmation_token) { user.confirmation_token } + + before do + user.confirm + subject + end + + it 'renders `new`' do + expect(response).to render_template(:new) + end + + it 'displays an error message' do + expect(response.body).to include('Email was already confirmed, please try signing in') + end + + it 'does not display the email of the user' do + expect(response.body).not_to include(user.email) + end + end + + context 'user accesses the link after the expiry of confirmation token has passed' do + let_it_be_with_reload(:user) { create(:user, :unconfirmed) } + let(:confirmation_token) { user.confirmation_token } + + before do + allow(Devise).to receive(:confirm_within).and_return(1.day) + + travel_to(3.days.from_now) do + subject + end + end + + it 'renders `new`' do + expect(response).to render_template(:new) + end + + it 'displays an error message' do + expect(response.body).to include('Email needs to be confirmed within 1 day, please request a new one below') + end + + it 'does not display the email of the user' do + expect(response.body).not_to include(user.email) + end + end + + context 'with an invalid confirmation token' do + let(:confirmation_token) { 'invalid_confirmation_token' } + + before do + subject + end + + it 'renders `new`' do + expect(response).to render_template(:new) + end + + it 'displays an error message' do + expect(response.body).to include('Confirmation token is invalid') + end + end + end +end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index bec4b24484a..2e57a901319 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -247,32 +247,99 @@ RSpec.describe UsersController do describe 'GET #contributed' do let(:project) { create(:project, :public) } - let(:current_user) { create(:user) } + + subject do + get :contributed, params: { username: author.username }, format: format + end before do - sign_in(current_user) + sign_in(user) project.add_developer(public_user) project.add_developer(private_user) + create(:push_event, project: project, author: author) + + subject end - context 'with public profile' do + shared_examples_for 'renders contributed projects' do it 'renders contributed projects' do - create(:push_event, project: project, author: public_user) + expect(assigns[:contributed_projects]).not_to be_empty + expect(response).to have_gitlab_http_status(:ok) + end + end - get :contributed, params: { username: public_user.username } + %i(html json).each do |format| + context "format: #{format}" do + let(:format) { format } - expect(assigns[:contributed_projects]).not_to be_empty + context 'with public profile' do + let(:author) { public_user } + + it_behaves_like 'renders contributed projects' + end + + context 'with private profile' do + let(:author) { private_user } + + it 'returns 404' do + expect(response).to have_gitlab_http_status(:not_found) + end + + context 'with a user that has the ability to read private profiles', :enable_admin_mode do + let(:user) { create(:admin) } + + it_behaves_like 'renders contributed projects' + end + end + end + end + end + + describe 'GET #starred' do + let(:project) { create(:project, :public) } + + subject do + get :starred, params: { username: author.username }, format: format + end + + before do + author.toggle_star(project) + + sign_in(user) + subject + end + + shared_examples_for 'renders starred projects' do + it 'renders starred projects' do + expect(response).to have_gitlab_http_status(:ok) + expect(assigns[:starred_projects]).not_to be_empty end end - context 'with private profile' do - it 'does not render contributed projects' do - create(:push_event, project: project, author: private_user) + %i(html json).each do |format| + context "format: #{format}" do + let(:format) { format } + + context 'with public profile' do + let(:author) { public_user } + + it_behaves_like 'renders starred projects' + end + + context 'with private profile' do + let(:author) { private_user } + + it 'returns 404' do + expect(response).to have_gitlab_http_status(:not_found) + end - get :contributed, params: { username: private_user.username } + context 'with a user that has the ability to read private profiles', :enable_admin_mode do + let(:user) { create(:admin) } - expect(assigns[:contributed_projects]).to be_empty + it_behaves_like 'renders starred projects' + end + end end end end diff --git a/spec/finders/starred_projects_finder_spec.rb b/spec/finders/starred_projects_finder_spec.rb index 15d4ae52ddd..f5d3314021d 100644 --- a/spec/finders/starred_projects_finder_spec.rb +++ b/spec/finders/starred_projects_finder_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe StarredProjectsFinder do let(:project1) { create(:project, :public, :empty_repo) } let(:project2) { create(:project, :public, :empty_repo) } - let(:other_project) { create(:project, :public, :empty_repo) } + let(:private_project) { create(:project, :private, :empty_repo) } let(:user) { create(:user) } let(:other_user) { create(:user) } @@ -13,6 +13,9 @@ RSpec.describe StarredProjectsFinder do before do user.toggle_star(project1) user.toggle_star(project2) + + private_project.add_maintainer(user) + user.toggle_star(private_project) end describe '#execute' do @@ -20,22 +23,56 @@ RSpec.describe StarredProjectsFinder do subject { finder.execute } - describe 'as same user' do - let(:current_user) { user } + context 'user has a public profile' do + describe 'as same user' do + let(:current_user) { user } - it { is_expected.to contain_exactly(project1, project2) } - end + it { is_expected.to contain_exactly(project1, project2, private_project) } + end + + describe 'as other user' do + let(:current_user) { other_user } - describe 'as other user' do - let(:current_user) { other_user } + it { is_expected.to contain_exactly(project1, project2) } + end - it { is_expected.to contain_exactly(project1, project2) } + describe 'as no user' do + let(:current_user) { nil } + + it { is_expected.to contain_exactly(project1, project2) } + end end - describe 'as no user' do - let(:current_user) { nil } + context 'user has a private profile' do + before do + user.update!(private_profile: true) + end + + describe 'as same user' do + let(:current_user) { user } + + it { is_expected.to contain_exactly(project1, project2, private_project) } + end + + describe 'as other user' do + context 'user does not have access to view the private profile' do + let(:current_user) { other_user } + + it { is_expected.to be_empty } + end + + context 'user has access to view the private profile', :enable_admin_mode do + let(:current_user) { create(:admin) } + + it { is_expected.to contain_exactly(project1, project2, private_project) } + end + end + + describe 'as no user' do + let(:current_user) { nil } - it { is_expected.to contain_exactly(project1, project2) } + it { is_expected.to be_empty } + end end end end diff --git a/spec/requests/api/graphql/user/starred_projects_query_spec.rb b/spec/requests/api/graphql/user/starred_projects_query_spec.rb index 8a1bd3d172f..b098058a735 100644 --- a/spec/requests/api/graphql/user/starred_projects_query_spec.rb +++ b/spec/requests/api/graphql/user/starred_projects_query_spec.rb @@ -70,4 +70,31 @@ RSpec.describe 'Getting starredProjects of the user' do ) end end + + context 'the user has a private profile' do + before do + user.update!(private_profile: true) + post_graphql(query, current_user: current_user) + end + + context 'the current user does not have access to view the private profile of the user' do + let(:current_user) { create(:user) } + + it 'finds no projects' do + expect(starred_projects).to be_empty + end + end + + context 'the current user has access to view the private profile of the user' do + let(:current_user) { create(:admin) } + + it 'finds all projects starred by the user, which the current user has access to' do + expect(starred_projects).to contain_exactly( + a_hash_including('id' => global_id_of(project_a)), + a_hash_including('id' => global_id_of(project_b)), + a_hash_including('id' => global_id_of(project_c)) + ) + end + end + end end diff --git a/spec/requests/api/graphql/user_query_spec.rb b/spec/requests/api/graphql/user_query_spec.rb index 0620d9ada9a..ec188424c43 100644 --- a/spec/requests/api/graphql/user_query_spec.rb +++ b/spec/requests/api/graphql/user_query_spec.rb @@ -238,7 +238,7 @@ RSpec.describe 'getting user information' do context 'the user is private' do before do - user.update(private_profile: true) + user.update!(private_profile: true) post_graphql(query, current_user: current_user) end @@ -248,6 +248,50 @@ RSpec.describe 'getting user information' do it_behaves_like 'a working graphql query' end + context 'we request the groupMemberships' do + let_it_be(:membership_a) { create(:group_member, :developer, user: user) } + let(:group_memberships) { graphql_data_at(:user, :group_memberships, :nodes) } + let(:user_fields) { 'groupMemberships { nodes { id } }' } + + it_behaves_like 'a working graphql query' + + it 'cannot be found' do + expect(group_memberships).to be_empty + end + + context 'the current user is the user' do + let(:current_user) { user } + + it 'can be found' do + expect(group_memberships).to include( + a_hash_including('id' => global_id_of(membership_a)) + ) + end + end + end + + context 'we request the projectMemberships' do + let_it_be(:membership_a) { create(:project_member, user: user) } + let(:project_memberships) { graphql_data_at(:user, :project_memberships, :nodes) } + let(:user_fields) { 'projectMemberships { nodes { id } }' } + + it_behaves_like 'a working graphql query' + + it 'cannot be found' do + expect(project_memberships).to be_empty + end + + context 'the current user is the user' do + let(:current_user) { user } + + it 'can be found' do + expect(project_memberships).to include( + a_hash_including('id' => global_id_of(membership_a)) + ) + end + end + end + context 'we request the authoredMergeRequests' do let(:user_fields) { 'authoredMergeRequests { nodes { id } }' } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 2abcb39a1c8..75d4758bb2a 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1255,13 +1255,46 @@ RSpec.describe API::Projects do expect(json_response['message']).to eq('404 User Not Found') end - it 'returns projects filtered by user' do - get api("/users/#{user3.id}/starred_projects/", user) + context 'with a public profile' do + it 'returns projects filtered by user' do + get api("/users/#{user3.id}/starred_projects/", user) - expect(response).to have_gitlab_http_status(:ok) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.map { |project| project['id'] }).to contain_exactly(project.id, project2.id, project3.id) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.map { |project| project['id'] }) + .to contain_exactly(project.id, project2.id, project3.id) + end + end + + context 'with a private profile' do + before do + user3.update!(private_profile: true) + user3.reload + end + + context 'user does not have access to view the private profile' do + it 'returns no projects' do + get api("/users/#{user3.id}/starred_projects/", user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response).to be_empty + end + end + + context 'user has access to view the private profile' do + it 'returns projects filtered by user' do + get api("/users/#{user3.id}/starred_projects/", admin) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.map { |project| project['id'] }) + .to contain_exactly(project.id, project2.id, project3.id) + end + end end end diff --git a/spec/validators/zoom_url_validator_spec.rb b/spec/validators/zoom_url_validator_spec.rb new file mode 100644 index 00000000000..7d5c94bc249 --- /dev/null +++ b/spec/validators/zoom_url_validator_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ZoomUrlValidator do + let(:zoom_meeting) { build(:zoom_meeting) } + + describe 'validations' do + context 'when zoom link starts with https' do + it 'passes validation' do + zoom_meeting.url = 'https://zoom.us/j/123456789' + + expect(zoom_meeting.valid?).to eq(true) + expect(zoom_meeting.errors).to be_empty + end + end + + shared_examples 'zoom link does not start with https' do |url| + it 'fails validation' do + zoom_meeting.url = url + expect(zoom_meeting.valid?).to eq(false) + + expect(zoom_meeting.errors).to be_present + expect(zoom_meeting.errors.first[1]).to eq 'must contain one valid Zoom URL' + end + end + + context 'when zoom link does not start with https' do + include_examples 'zoom link does not start with https', 'http://zoom.us/j/123456789' + + context 'when zoom link does not start with a scheme' do + include_examples 'zoom link does not start with https', 'testinghttp://zoom.us/j/123456789' + end + end + end +end diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100644..100755 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100644..100755 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |