Add latest changes from gitlab-org/security/gitlab@13-6-stable-ee

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
 

app/finders/starred_projects_finder.rb

 # 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

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')

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

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: 'gl-button btn btn-success'
 

changelogs/unreleased/security-hide-email-in-confirmation-page.yml

+---
+title: Do not show emails of users in confirmation page
+merge_request:
+author:
+type: security

changelogs/unreleased/security-project-import-zoom-xss.yml

+---
+title: Validate zoom links to start with https only
+merge_request: 1055
+author:
+type: security

changelogs/unreleased/security-starred-projects-api-fix.yml

+---
+title: Do not expose starred projects of users with private profile via API
+merge_request:
+author:
+type: security

changelogs/unreleased/security-starred-projects-private-profile.yml

+---
+title: Do not show starred & contributed projects of users with private profile
+merge_request:
+author:
+type: security

config/feature_categories.yml

@@ -45,6 +45,7 @@
 - dynamic_application_security_testing
 - editor_extension
 - epics
+- epic_tracking
 - error_tracking
 - feature_flags
 - foundations

db/post_migrate/20201109114603_schedule_remove_inaccessible_epic_todos.rb

+# 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

db/schema_migrations/20201109114603

+ae8034ec52df47ce2ce3397715dd18347e4d297a963c17c7b26321f414dfa632
\ No newline at end of file

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.
 

lib/gitlab/background_migration/remove_inaccessible_epic_todos.rb

+# 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')

spec/controllers/confirmations_controller_spec.rb

+# 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

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
+
+    %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
-      it 'does not render contributed projects' do
-        create(:push_event, project: project, author: private_user)
+          let(:author) { private_user }
 
-        get :contributed, params: { username: private_user.username }
+          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) }
 
-        expect(assigns[:contributed_projects]).to be_empty
+            it_behaves_like 'renders starred projects'
+          end
+        end
       end
     end
   end

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,10 +23,11 @@ RSpec.describe StarredProjectsFinder do
 
     subject { finder.execute }
 
+    context 'user has a public profile' do
       describe 'as same user' do
         let(:current_user) { user }
 
-      it { is_expected.to contain_exactly(project1, project2) }
+        it { is_expected.to contain_exactly(project1, project2, private_project) }
       end
 
       describe 'as other user' do
@@ -38,4 +42,37 @@ RSpec.describe StarredProjectsFinder do
         it { is_expected.to contain_exactly(project1, project2) }
       end
     end
+
+    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 be_empty }
+      end
+    end
+  end
 end

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

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
 
+    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(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
 

spec/validators/zoom_url_validator_spec.rb

+# 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