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

12 files changed, 182 insertions, 5 deletions

diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
index 0c0548a17a1..a7424c610c9 100644
--- a/app/services/todos/destroy/entity_leave_service.rb
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -24,7 +24,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
@@ -43,7 +43,7 @@ module Todos
       end
 
       # rubocop: disable CodeReuse/ActiveRecord
-      def remove_confidential_issue_todos
+      def remove_confidential_resource_todos
         Todo.where(
           target_id: confidential_issues.select(:id), target_type: Issue.name, user_id: user.id
         ).delete_all
@@ -147,3 +147,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-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/config/feature_categories.yml b/config/feature_categories.yml
index 7b85f910d85..5c6c11f01ea 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 1fca3c0ab64..d2a9e1e2df0 100644
--- a/doc/user/todos.md
+++ b/doc/user/todos.md
@@ -64,7 +64,7 @@ To-do triggers aren't affected by [GitLab notification email settings](profile/n
 
 NOTE: **Note:**
 When a user no longer has access to a resource related to a to-do (such as an
-issue, merge request, project, or group), for security reasons GitLab deletes
+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/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