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

13 files changed, 158 insertions, 56 deletions

diff --git a/app/channels/application_cable/connection.rb b/app/channels/application_cable/connection.rb
index 1361269f2a2..b2d98d243f9 100644
--- a/app/channels/application_cable/connection.rb
+++ b/app/channels/application_cable/connection.rb
@@ -15,7 +15,7 @@ module ApplicationCable
     private
 
     def find_user_from_session_store
-      session = ActiveSession.sessions_from_ids([session_id]).first
+      session = ActiveSession.sessions_from_ids([session_id.private_id]).first
       Warden::SessionSerializer.new('rack.session' => session).fetch(:user)
     end
 
diff --git a/app/controllers/profiles/active_sessions_controller.rb b/app/controllers/profiles/active_sessions_controller.rb
index d9ec3195fd1..e4cd5d65e1a 100644
--- a/app/controllers/profiles/active_sessions_controller.rb
+++ b/app/controllers/profiles/active_sessions_controller.rb
@@ -6,7 +6,9 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController
   end
 
   def destroy
-    ActiveSession.destroy_with_public_id(current_user, params[:id])
+    # params[:id] can be either an Rack::Session::SessionId#private_id
+    # or an encrypted Rack::Session::SessionId#public_id
+    ActiveSession.destroy_with_deprecated_encryption(current_user, params[:id])
     current_user.forget_me!
 
     respond_to do |format|
diff --git a/app/helpers/active_sessions_helper.rb b/app/helpers/active_sessions_helper.rb
index b80152777a8..322c5b3b16d 100644
--- a/app/helpers/active_sessions_helper.rb
+++ b/app/helpers/active_sessions_helper.rb
@@ -22,4 +22,13 @@ module ActiveSessionsHelper
 
     sprite_icon(icon_name, css_class: 'gl-mt-2')
   end
+
+  def revoke_session_path(active_session)
+    if active_session.session_private_id
+      profile_active_session_path(active_session.session_private_id)
+    else
+      # TODO: remove in 13.7
+      profile_active_session_path(active_session.public_id)
+    end
+  end
 end
diff --git a/app/models/active_session.rb b/app/models/active_session.rb
index 4908290e06b..dded0eb1dc3 100644
--- a/app/models/active_session.rb
+++ b/app/models/active_session.rb
@@ -9,14 +9,14 @@ class ActiveSession
   attr_accessor :created_at, :updated_at,
     :ip_address, :browser, :os,
     :device_name, :device_type,
-    :is_impersonated, :session_id
+    :is_impersonated, :session_id, :session_private_id
 
-  def current?(session)
-    return false if session_id.nil? || session.id.nil?
+  def current?(rack_session)
+    return false if session_private_id.nil? || rack_session.id.nil?
 
     # Rack v2.0.8+ added private_id, which uses the hash of the
     # public_id to avoid timing attacks.
-    session_id.private_id == session.id.private_id
+    session_private_id == rack_session.id.private_id
   end
 
   def human_device_type
@@ -25,13 +25,14 @@ class ActiveSession
 
   # This is not the same as Rack::Session::SessionId#public_id, but we
   # need to preserve this for backwards compatibility.
+  # TODO: remove in 13.7
   def public_id
-    Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id.public_id)
+    Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
   end
 
   def self.set(user, request)
     Gitlab::Redis::SharedState.with do |redis|
-      session_id = request.session.id.public_id
+      session_private_id = request.session.id.private_id
       client = DeviceDetector.new(request.user_agent)
       timestamp = Time.current
 
@@ -43,25 +44,37 @@ class ActiveSession
         device_type: client.device_type,
         created_at: user.current_sign_in_at || timestamp,
         updated_at: timestamp,
-        session_id: session_id,
+        # TODO: remove in 13.7
+        session_id: request.session.id.public_id,
+        session_private_id: session_private_id,
         is_impersonated: request.session[:impersonator_id].present?
       )
 
       redis.pipelined do
         redis.setex(
-          key_name(user.id, session_id),
+          key_name(user.id, session_private_id),
           Settings.gitlab['session_expire_delay'] * 60,
           Marshal.dump(active_user_session)
         )
 
         redis.sadd(
           lookup_key_name(user.id),
-          session_id
+          session_private_id
         )
+
+        # We remove the ActiveSession stored by using public_id to avoid
+        # duplicate entries
+        remove_deprecated_active_sessions_with_public_id(redis, user.id, request.session.id.public_id)
       end
     end
   end
 
+  # TODO: remove in 13.7
+  private_class_method def self.remove_deprecated_active_sessions_with_public_id(redis, user_id, rack_session_public_id)
+    redis.srem(lookup_key_name(user_id), rack_session_public_id)
+    redis.del(key_name(user_id, rack_session_public_id))
+  end
+
   def self.list(user)
     Gitlab::Redis::SharedState.with do |redis|
       cleaned_up_lookup_entries(redis, user).map do |raw_session|
@@ -70,27 +83,29 @@ class ActiveSession
     end
   end
 
-  def self.destroy(user, session_id)
-    return unless session_id
-
+  def self.cleanup(user)
     Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, [session_id])
+      clean_up_old_sessions(redis, user)
+      cleaned_up_lookup_entries(redis, user)
     end
   end
 
-  def self.destroy_with_public_id(user, public_id)
-    decrypted_id = decrypt_public_id(public_id)
+  # TODO: remove in 13.7
+  # After upgrade there might be a duplicate ActiveSessions:
+  # - one with the public_id stored in #session_id
+  # - another with private_id stored in #session_private_id
+  def self.destroy_with_rack_session_id(user, rack_session_id)
+    return unless rack_session_id
 
-    return if decrypted_id.nil?
-
-    session_id = Rack::Session::SessionId.new(decrypted_id)
-    destroy(user, session_id)
+    Gitlab::Redis::SharedState.with do |redis|
+      destroy_sessions(redis, user, [rack_session_id.public_id, rack_session_id.private_id])
+    end
   end
 
   def self.destroy_sessions(redis, user, session_ids)
-    key_names = session_ids.map { |session_id| key_name(user.id, session_id.public_id) }
+    key_names = session_ids.map { |session_id| key_name(user.id, session_id) }
 
-    redis.srem(lookup_key_name(user.id), session_ids.map(&:public_id))
+    redis.srem(lookup_key_name(user.id), session_ids)
 
     Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
       redis.del(key_names)
@@ -98,19 +113,29 @@ class ActiveSession
     end
   end
 
-  def self.cleanup(user)
+  # TODO: remove in 13.7
+  # After upgrade, .destroy might be called with the session id encrypted
+  # by .public_id.
+  def self.destroy_with_deprecated_encryption(user, session_id)
+    return unless session_id
+
+    decrypted_session_id = decrypt_public_id(session_id)
+    rack_session_private_id = if decrypted_session_id
+                                Rack::Session::SessionId.new(decrypted_session_id).private_id
+                              end
+
     Gitlab::Redis::SharedState.with do |redis|
-      clean_up_old_sessions(redis, user)
-      cleaned_up_lookup_entries(redis, user)
+      destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact)
     end
   end
 
-  def self.destroy_all_but_current(user, current_session)
-    session_ids = not_impersonated(user)
-    session_ids.reject! { |session| session.current?(current_session) } if current_session
+  def self.destroy_all_but_current(user, current_rack_session)
+    sessions = not_impersonated(user)
+    sessions.reject! { |session| session.current?(current_rack_session) } if current_rack_session
 
     Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, session_ids.map(&:session_id)) if session_ids.any?
+      session_ids = (sessions.map(&:session_id) | sessions.map(&:session_private_id)).compact
+      destroy_sessions(redis, user, session_ids) if session_ids.any?
     end
   end
 
@@ -132,17 +157,16 @@ class ActiveSession
 
   # Lists the relevant session IDs for the user.
   #
-  # Returns an array of Rack::Session::SessionId objects
+  # Returns an array of strings
   def self.session_ids_for_user(user_id)
     Gitlab::Redis::SharedState.with do |redis|
-      session_ids = redis.smembers(lookup_key_name(user_id))
-      session_ids.map { |id| Rack::Session::SessionId.new(id) }
+      redis.smembers(lookup_key_name(user_id))
     end
   end
 
   # Lists the session Hash objects for the given session IDs.
   #
-  # session_ids - An array of Rack::Session::SessionId objects
+  # session_ids - An array of strings
   #
   # Returns an array of ActiveSession objects
   def self.sessions_from_ids(session_ids)
@@ -168,27 +192,12 @@ class ActiveSession
   # Returns an ActiveSession object
   def self.load_raw_session(raw_session)
     # rubocop:disable Security/MarshalLoad
-    session = Marshal.load(raw_session)
+    Marshal.load(raw_session)
     # rubocop:enable Security/MarshalLoad
+  end
 
-    # Older ActiveSession models serialize `session_id` as strings, To
-    # avoid breaking older sessions, we keep backwards compatibility
-    # with older Redis keys and initiate Rack::Session::SessionId here.
-    session.session_id = Rack::Session::SessionId.new(session.session_id) if session.try(:session_id).is_a?(String)
-    session
-  end
-
-  def self.rack_session_keys(session_ids)
-    session_ids.each_with_object([]) do |session_id, arr|
-      # This is a redis-rack implementation detail
-      # (https://github.com/redis-store/redis-rack/blob/master/lib/rack/session/redis.rb#L88)
-      #
-      # We need to delete session keys based on the legacy public key name
-      # and the newer private ID keys, but there's no well-defined interface
-      # so we have to do it directly.
-      arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.public_id}"
-      arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.private_id}"
-    end
+  def self.rack_session_keys(rack_session_ids)
+    rack_session_ids.map { |session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
   end
 
   def self.raw_active_session_entries(redis, session_ids, user_id)
@@ -220,7 +229,7 @@ class ActiveSession
     sessions = active_session_entries(session_ids, user.id, redis)
     sessions.sort_by! {|session| session.updated_at }.reverse!
     destroyable_sessions = sessions.drop(ALLOWED_NUMBER_OF_ACTIVE_SESSIONS)
-    destroyable_session_ids = destroyable_sessions.map { |session| session.session_id }
+    destroyable_session_ids = destroyable_sessions.flat_map { |session| [session.session_id, session.session_private_id] }.compact
     destroy_sessions(redis, user, destroyable_session_ids) if destroyable_session_ids.any?
   end
 
@@ -244,6 +253,7 @@ class ActiveSession
     entries.compact
   end
 
+  # TODO: remove in 13.7
   private_class_method def self.decrypt_public_id(public_id)
     Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id)
   rescue
diff --git a/app/models/member.rb b/app/models/member.rb
index 5a084a3a2e6..7ea9caa45d3 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -5,6 +5,7 @@ class Member < ApplicationRecord
   include AfterCommitQueue
   include Sortable
   include Importable
+  include CreatedAtFilterable
   include Expirable
   include Gitlab::Access
   include Presentable
@@ -20,6 +21,7 @@ class Member < ApplicationRecord
 
   delegate :name, :username, :email, to: :user, prefix: true
 
+  validates :expires_at, allow_blank: true, future_date: true
   validates :user, presence: true, unless: :invite?
   validates :source, presence: true
   validates :user_id, uniqueness: { scope: [:source_type, :source_id],
diff --git a/app/models/operations/feature_flag.rb b/app/models/operations/feature_flag.rb
index 586e9d689a1..104338b80d1 100644
--- a/app/models/operations/feature_flag.rb
+++ b/app/models/operations/feature_flag.rb
@@ -4,8 +4,11 @@ module Operations
   class FeatureFlag < ApplicationRecord
     include AtomicInternalId
     include IidRoutes
+    include Limitable
 
     self.table_name = 'operations_feature_flags'
+    self.limit_scope = :project
+    self.limit_name = 'project_feature_flags'
 
     belongs_to :project
 
diff --git a/app/models/releases/link.rb b/app/models/releases/link.rb
index e1dc3b904b9..82272f4857a 100644
--- a/app/models/releases/link.rb
+++ b/app/models/releases/link.rb
@@ -6,7 +6,9 @@ module Releases
 
     belongs_to :release
 
-    FILEPATH_REGEX = %r{\A/(?:[\-\.\w]+/?)*[\da-zA-Z]+\z}.freeze
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/218753
+    # Regex modified to prevent catastrophic backtracking
+    FILEPATH_REGEX = %r{\A\/[^\/](?!.*\/\/.*)[\-\.\w\/]+[\da-zA-Z]+\z}.freeze
 
     validates :url, presence: true, addressable_url: { schemes: %w(http https ftp) }, uniqueness: { scope: :release }
     validates :name, presence: true, uniqueness: { scope: :release }
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 56bcef0c562..1672ba2830a 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -19,6 +19,7 @@ class IssuableBaseService < BaseService
 
   def filter_params(issuable)
     unless can_admin_issuable?(issuable)
+      params.delete(:milestone)
       params.delete(:milestone_id)
       params.delete(:labels)
       params.delete(:add_label_ids)
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index ce21b2e0275..b9832400302 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -3,6 +3,7 @@
 module Issues
   class UpdateService < Issues::BaseService
     include SpamCheckMethods
+    extend ::Gitlab::Utils::Override
 
     def execute(issue)
       handle_move_between_ids(issue)
@@ -17,6 +18,17 @@ module Issues
       super
     end
 
+    override :filter_params
+    def filter_params(issue)
+      super
+
+      # filter confidential in `Issues::UpdateService` and not in `IssuableBaseService#filtr_params`
+      # because we do allow users that cannot admin issues to set confidential flag when creating an issue
+      unless can_admin_issuable?(issue)
+        params.delete(:confidential)
+      end
+    end
+
     def before_update(issue, skip_spam_check: false)
       spam_check(issue, current_user, action: :update) unless skip_spam_check
     end
diff --git a/app/validators/future_date_validator.rb b/app/validators/future_date_validator.rb
new file mode 100644
index 00000000000..1ba4a1e273d
--- /dev/null
+++ b/app/validators/future_date_validator.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+# FutureDateValidator
+
+# Validates that a date is in the future.
+#
+# Example:
+#
+#   class Member < ActiveRecord::Base
+#     validates :expires_at, allow_blank: true, future_date: true
+#   end
+
+class FutureDateValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    record.errors.add(attribute, _('cannot be a date in the past')) if value < Date.current
+  end
+end
diff --git a/app/views/profiles/active_sessions/_active_session.html.haml b/app/views/profiles/active_sessions/_active_session.html.haml
index c4b3fa80d75..97f13a55dea 100644
--- a/app/views/profiles/active_sessions/_active_session.html.haml
+++ b/app/views/profiles/active_sessions/_active_session.html.haml
@@ -27,6 +27,9 @@
 
   - unless is_current_session
     .float-right
-      = link_to profile_active_session_path(active_session.public_id), data: { confirm: _('Are you sure? The device will be signed out of GitLab and all remember me tokens revoked.') }, method: :delete, class: "btn btn-danger gl-ml-3" do
+      = link_to(revoke_session_path(active_session),
+                { data: { confirm: _('Are you sure? The device will be signed out of GitLab and all remember me tokens revoked.') },
+                  method: :delete,
+                  class: "btn btn-danger gl-ml-3" }) do
         %span.sr-only= _('Revoke')
         = _('Revoke')
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 451decce9fb..11bf797fb90 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -315,6 +315,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: cronjob:remove_unaccepted_member_invites
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: cronjob:remove_unreferenced_lfs_objects
   :feature_category: :git_lfs
   :has_external_dependencies:
diff --git a/app/workers/remove_unaccepted_member_invites_worker.rb b/app/workers/remove_unaccepted_member_invites_worker.rb
new file mode 100644
index 00000000000..4b75b43791e
--- /dev/null
+++ b/app/workers/remove_unaccepted_member_invites_worker.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+class RemoveUnacceptedMemberInvitesWorker # rubocop:disable Scalability/IdempotentWorker
+  include ApplicationWorker
+  include CronjobQueue # rubocop:disable Scalability/CronWorkerContext
+
+  feature_category :authentication_and_authorization
+  urgency :low
+  idempotent!
+
+  EXPIRATION_THRESHOLD = 90.days
+  BATCH_SIZE = 10_000
+
+  def perform
+    # We need to check for user_id IS NULL because we have accepted invitations
+    # in the database where we did not clear the invite_token. We do not
+    # want to accidentally delete those members.
+    loop do
+      # rubocop: disable CodeReuse/ActiveRecord
+      inner_query = Member
+                      .select(:id)
+                      .invite
+                      .created_before(EXPIRATION_THRESHOLD.ago)
+                      .where(user_id: nil)
+                      .limit(BATCH_SIZE)
+
+      records_deleted = Member.where(id: inner_query).delete_all
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      break if records_deleted == 0
+    end
+  end
+end