Merge branch 'master' of dev.gitlab.org:gitlab/gitlab-ee

CHANGELOG-EE.md

 Please view this file on the master branch, on stable branches it's out of date.
 
+## 12.9.3 (2020-04-14)
+
+### Security (1 change)
+
+- Fix filename bypass when uploading NuGet packages.
+
+
 ## 12.9.2 (2020-03-31)
 
 ### Fixed (4 changes)
@@ -152,6 +159,13 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Allow users to be marked as service users. !202680
 
 
+## 12.8.9 (2020-04-14)
+
+### Security (1 change)
+
+- Fix filename bypass when uploading NuGet packages.
+
+
 ## 12.8.7 (2020-03-16)
 
 ### Fixed (1 change)
@@ -305,6 +319,13 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Prepare DB structure for GMA forking changes. !22002
 
 
+## 12.7.9 (2020-04-14)
+
+### Security (1 change)
+
+- Fix filename bypass when uploading NuGet packages.
+
+
 ## 12.7.5
 
 ### Fixed (1 change)

CHANGELOG.md

@@ -2,6 +2,15 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 12.9.3 (2020-04-14)
+
+### Security (3 changes)
+
+- Refresh ProjectAuthorization during Group deletion.
+- Prevent filename bypass on artifact upload.
+- Update rack and related gems to 2.0.9 to fix security issue.
+
+
 ## 12.9.2 (2020-03-31)
 
 ### Fixed (5 changes)
@@ -499,6 +508,15 @@ entry.
 - Improvement in token reference.
 
 
+## 12.8.9 (2020-04-14)
+
+### Security (3 changes)
+
+- Refresh ProjectAuthorization during Group deletion.
+- Prevent filename bypass on artifact upload.
+- Update rack and related gems to 2.0.9 to fix security issue.
+
+
 ## 12.8.7 (2020-03-16)
 
 ### Fixed (1 change, 1 of them is from the community)
@@ -1089,6 +1107,15 @@ entry.
 - Add clarifying content to account fields.
 
 
+## 12.7.9 (2020-04-14)
+
+### Security (3 changes)
+
+- Refresh ProjectAuthorization during Group deletion.
+- Prevent filename bypass on artifact upload.
+- Update rack and related gems to 2.0.9 to fix security issue.
+
+
 ## 12.7.5
 
 ### Fixed (4 changes, 1 of them is from the community)

GITLAB_WORKHORSE_VERSION

-8.30.0
+8.30.1

Gemfile

@@ -163,7 +163,7 @@ gem 'diffy', '~> 3.3'
 gem 'diff_match_patch', '~> 0.1.0'
 
 # Application server
-gem 'rack', '~> 2.0.7'
+gem 'rack', '~> 2.0.9'
 
 group :unicorn do
   gem 'unicorn', '~> 5.4.1'

Gemfile.lock

@@ -173,7 +173,7 @@ GEM
     concord (0.1.5)
       adamantium (~> 0.2.0)
       equalizer (~> 0.0.9)
-    concurrent-ruby (1.1.5)
+    concurrent-ruby (1.1.6)
     connection_pool (2.2.2)
     contracts (0.11.0)
     cork (0.3.0)
@@ -788,7 +788,7 @@ GEM
     public_suffix (4.0.3)
     pyu-ruby-sasl (0.0.3.3)
     raabro (1.1.6)
-    rack (2.0.7)
+    rack (2.0.9)
     rack-accept (0.4.5)
       rack (>= 0.4)
     rack-attack (6.2.0)
@@ -859,17 +859,17 @@ GEM
       json
     recursive-open-struct (1.1.0)
     redis (4.1.3)
-    redis-actionpack (5.1.0)
-      actionpack (>= 4.0, < 7)
-      redis-rack (>= 1, < 3)
+    redis-actionpack (5.2.0)
+      actionpack (>= 5, < 7)
+      redis-rack (>= 2.1.0, < 3)
       redis-store (>= 1.1.0, < 2)
     redis-activesupport (5.2.0)
       activesupport (>= 3, < 7)
       redis-store (>= 1.3, < 2)
     redis-namespace (1.6.0)
       redis (>= 3.0.4)
-    redis-rack (2.0.6)
-      rack (>= 1.5, < 3)
+    redis-rack (2.1.2)
+      rack (>= 2.0.8, < 3)
       redis-store (>= 1.2, < 2)
     redis-rails (5.0.2)
       redis-actionpack (>= 5.0, < 6)
@@ -1331,7 +1331,7 @@ DEPENDENCIES
   prometheus-client-mmap (~> 0.10.0)
   pry-byebug (~> 3.5.1)
   pry-rails (~> 0.3.9)
-  rack (~> 2.0.7)
+  rack (~> 2.0.9)
   rack-attack (~> 6.2.0)
   rack-cors (~> 1.0.6)
   rack-oauth2 (~> 1.9.3)

app/models/active_session.rb

@@ -6,31 +6,32 @@ class ActiveSession
   SESSION_BATCH_SIZE = 200
   ALLOWED_NUMBER_OF_ACTIVE_SESSIONS = 100
 
-  attr_writer :session_id
-
   attr_accessor :created_at, :updated_at,
     :ip_address, :browser, :os,
     :device_name, :device_type,
-    :is_impersonated
+    :is_impersonated, :session_id
 
   def current?(session)
     return false if session_id.nil? || session.id.nil?
 
-    session_id == session.id
+    # 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
   end
 
   def human_device_type
     device_type&.titleize
   end
 
+  # This is not the same as Rack::Session::SessionId#public_id, but we
+  # need to preserve this for backwards compatibility.
   def public_id
-    encrypted_id = Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
-    CGI.escape(encrypted_id)
+    Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id.public_id)
   end
 
   def self.set(user, request)
     Gitlab::Redis::SharedState.with do |redis|
-      session_id = request.session.id
+      session_id = request.session.id.public_id
       client = DeviceDetector.new(request.user_agent)
       timestamp = Time.current
 
@@ -63,32 +64,35 @@ class ActiveSession
 
   def self.list(user)
     Gitlab::Redis::SharedState.with do |redis|
-      cleaned_up_lookup_entries(redis, user).map do |entry|
-        # rubocop:disable Security/MarshalLoad
-        Marshal.load(entry)
-        # rubocop:enable Security/MarshalLoad
+      cleaned_up_lookup_entries(redis, user).map do |raw_session|
+        load_raw_session(raw_session)
       end
     end
   end
 
   def self.destroy(user, session_id)
+    return unless session_id
+
     Gitlab::Redis::SharedState.with do |redis|
       destroy_sessions(redis, user, [session_id])
     end
   end
 
   def self.destroy_with_public_id(user, public_id)
-    session_id = decrypt_public_id(public_id)
-    destroy(user, session_id) unless session_id.nil?
+    decrypted_id = decrypt_public_id(public_id)
+
+    return if decrypted_id.nil?
+
+    session_id = Rack::Session::SessionId.new(decrypted_id)
+    destroy(user, session_id)
   end
 
   def self.destroy_sessions(redis, user, session_ids)
-    key_names = session_ids.map {|session_id| key_name(user.id, session_id) }
-    session_names = session_ids.map {|session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
+    key_names = session_ids.map { |session_id| key_name(user.id, session_id.public_id) }
 
-    redis.srem(lookup_key_name(user.id), session_ids)
+    redis.srem(lookup_key_name(user.id), session_ids.map(&:public_id))
     redis.del(key_names)
-    redis.del(session_names)
+    redis.del(rack_session_keys(session_ids))
   end
 
   def self.cleanup(user)
@@ -110,28 +114,65 @@ class ActiveSession
     sessions_from_ids(session_ids_for_user(user.id))
   end
 
+  # Lists the relevant session IDs for the user.
+  #
+  # Returns an array of Rack::Session::SessionId objects
   def self.session_ids_for_user(user_id)
     Gitlab::Redis::SharedState.with do |redis|
-      redis.smembers(lookup_key_name(user_id))
+      session_ids = redis.smembers(lookup_key_name(user_id))
+      session_ids.map { |id| Rack::Session::SessionId.new(id) }
     end
   end
 
+  # Lists the ActiveSession objects for the given session IDs.
+  #
+  # session_ids - An array of Rack::Session::SessionId objects
+  #
+  # Returns an array of ActiveSession objects
   def self.sessions_from_ids(session_ids)
     return [] if session_ids.empty?
 
     Gitlab::Redis::SharedState.with do |redis|
-      session_keys = session_ids.map { |session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
+      session_keys = rack_session_keys(session_ids)
 
       session_keys.each_slice(SESSION_BATCH_SIZE).flat_map do |session_keys_batch|
         redis.mget(session_keys_batch).compact.map do |raw_session|
-          # rubocop:disable Security/MarshalLoad
-          Marshal.load(raw_session)
-          # rubocop:enable Security/MarshalLoad
+          load_raw_session(raw_session)
         end
       end
     end
   end
 
+  # Deserializes an ActiveSession object from Redis.
+  #
+  # raw_session - Raw bytes from Redis
+  #
+  # Returns an ActiveSession object
+  def self.load_raw_session(raw_session)
+    # rubocop:disable Security/MarshalLoad
+    session = Marshal.load(raw_session)
+    # rubocop:enable Security/MarshalLoad
+
+    # 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
+  end
+
   def self.raw_active_session_entries(redis, session_ids, user_id)
     return [] if session_ids.empty?
 
@@ -146,7 +187,7 @@ class ActiveSession
     entry_keys = raw_active_session_entries(redis, session_ids, user_id)
 
     entry_keys.compact.map do |raw_session|
-      Marshal.load(raw_session) # rubocop:disable Security/MarshalLoad
+      load_raw_session(raw_session)
     end
   end
 
@@ -159,10 +200,13 @@ 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.send :session_id } # rubocop:disable GitlabSecurity/PublicSend
+    destroyable_session_ids = destroyable_sessions.map { |session| session.session_id }
     destroy_sessions(redis, user, destroyable_session_ids) if destroyable_session_ids.any?
   end
 
+  # Cleans up the lookup set by removing any session IDs that are no longer present.
+  #
+  # Returns an array of marshalled ActiveModel objects that are still active.
   def self.cleaned_up_lookup_entries(redis, user)
     session_ids = session_ids_for_user(user.id)
     entries = raw_active_session_entries(redis, session_ids, user.id)
@@ -181,13 +225,8 @@ class ActiveSession
   end
 
   private_class_method def self.decrypt_public_id(public_id)
-    decoded_id = CGI.unescape(public_id)
-    Gitlab::CryptoHelper.aes256_gcm_decrypt(decoded_id)
+    Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id)
   rescue
     nil
   end
-
-  private
-
-  attr_reader :session_id
 end

app/services/groups/destroy_service.rb

@@ -29,7 +29,15 @@ module Groups
 
       group.chat_team&.remove_mattermost_team(current_user)
 
+      user_ids_for_project_authorizations_refresh = group.user_ids_for_project_authorizations
+
       group.destroy
+
+      UserProjectAccessChangedService
+        .new(user_ids_for_project_authorizations_refresh)
+        .execute(blocking: true)
+
+      group
     end
     # rubocop: enable CodeReuse/ActiveRecord
   end

db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb

+# frozen_string_literal: true
+
+class ScheduleRecalculateProjectAuthorizationsThirdRun < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'RecalculateProjectAuthorizationsWithMinMaxUserId'
+  BATCH_SIZE = 2_500
+  DELAY_INTERVAL = 2.minutes.to_i
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'users'
+  end
+
+  def up
+    say "Scheduling #{MIGRATION} jobs"
+
+    queue_background_migration_jobs_by_range_at_intervals(User, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+  end
+
+  def down
+  end
+end

db/structure.sql

@@ -12947,6 +12947,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200204070729
 20200204113223
 20200204113224
+20200204113225
 20200204131054
 20200204131831
 20200205143231

ee/lib/api/nuget_packages.rb

@@ -78,13 +78,13 @@ module API
           detail 'This feature was introduced in GitLab 12.6'
         end
         params do
-          use :workhorse_upload_params
+          requires :package, type: ::API::Validations::Types::WorkhorseFile, desc: 'The package file to be published (generated by Multipart middleware)'
         end
         put do
           authorize_upload!(authorized_user_project)
 
           file_params = params.merge(
-            file: uploaded_package_file(:package),
+            file: params[:package],
             file_name: PACKAGE_FILENAME
           )
 

ee/spec/lib/gitlab/middleware/multipart_spec.rb

@@ -27,12 +27,18 @@ describe Gitlab::Middleware::Multipart do
         end
       end
 
-      RSpec.shared_examples 'not allowing the multipart upload' do
+      RSpec.shared_examples 'not allowing the multipart upload when package upload path is used' do
         it 'does not allow files to be uploaded' do
           with_tmp_dir('tmp/uploads', storage_path) do |dir, env|
-            allow(Packages::PackageFileUploader).to receive(:root).and_return(File.join(dir, storage_path))
+            # with_tmp_dir set the same workhorse_upload_path for all Uploaders,
+            # so we have to prevent JobArtifactUploader to allow the tested path
+            allow(JobArtifactUploader).to receive(:workhorse_upload_path).and_return(Dir.tmpdir)
+
+            status, headers, body = middleware.call(env)
 
-            expect { middleware.call(env) }.to raise_error(UploadedFile::InvalidPathError)
+            expect(status).to eq(400)
+            expect(headers).to eq({ 'Content-Type' => 'text/plain' })
+            expect(body).to start_with('insecure path used')
           end
         end
       end
@@ -50,7 +56,7 @@ describe Gitlab::Middleware::Multipart do
           expect(::Packages::PackageFileUploader).not_to receive(:workhorse_upload_path)
         end
 
-        it_behaves_like 'not allowing the multipart upload'
+        it_behaves_like 'not allowing the multipart upload when package upload path is used'
       end
 
       where(:object_storage_enabled, :direct_upload_enabled, :example_name) do

ee/spec/requests/api/nuget_packages_spec.rb

@@ -130,14 +130,17 @@ describe API::NugetPackages do
     let(:url) { "/projects/#{project.id}/packages/nuget" }
     let(:headers) { {} }
     let(:params) { { package: temp_file(file_name) } }
+    let(:file_key) { :package }
+    let(:send_rewritten_field) { true }
 
     subject do
       workhorse_finalize(
         api(url),
         method: :put,
-        file_key: :package,
+        file_key: file_key,
         params: params,
-        headers: headers
+        headers: headers,
+        send_rewritten_field: send_rewritten_field
       )
     end
 

ee/spec/support/shared_examples/requests/api/nuget_packages_shared_examples.rb

@@ -171,7 +171,7 @@ RSpec.shared_examples 'process nuget upload' do |user_type, status, add_member =
       end
 
       context 'without a file from workhorse' do
-        let(:params) { { package: nil } }
+        let(:send_rewritten_field) { false }
 
         it_behaves_like 'returning response status', :bad_request
       end
@@ -212,6 +212,19 @@ RSpec.shared_examples 'process nuget upload' do |user_type, status, add_member =
             it_behaves_like 'returning response status', :forbidden
           end
         end
+
+        context 'with crafted package.path param' do
+          let(:crafted_file) { Tempfile.new('nuget.crafted.package.path') }
+          let(:url) { "/projects/#{project.id}/packages/nuget?package.path=#{crafted_file.path}" }
+          let(:params) { { file: temp_file(file_name) } }
+          let(:file_key) { :file }
+
+          it 'does not create a package file' do
+            expect { subject }.to change { ::Packages::PackageFile.count }.by(0)
+          end
+
+          it_behaves_like 'returning response status', :bad_request
+        end
       end
 
       context 'and direct upload disabled' do

lib/api/runner.rb

@@ -251,21 +251,14 @@ module API
       end
       params do
         requires :id, type: Integer, desc: %q(Job's ID)
+        requires :file, type: ::API::Validations::Types::WorkhorseFile, desc: %(The artifact file to store (generated by Multipart middleware))
         optional :token, type: String, desc: %q(Job's authentication token)
         optional :expire_in, type: String, desc: %q(Specify when artifacts should expire)
         optional :artifact_type, type: String, desc: %q(The type of artifact),
                                  default: 'archive', values: Ci::JobArtifact.file_types.keys
         optional :artifact_format, type: String, desc: %q(The format of artifact),
                                    default: 'zip', values: Ci::JobArtifact.file_formats.keys
-        optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse))
-        optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse))
-        optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse))
-        optional 'file.size', type: Integer, desc: %q(real size of file (generated by Workhorse))
-        optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file (generated by Workhorse))
-        optional 'metadata.path', type: String, desc: %q(path to locally stored body (generated by Workhorse))
-        optional 'metadata.name', type: String, desc: %q(filename (generated by Workhorse))
-        optional 'metadata.size', type: Integer, desc: %q(real size of metadata (generated by Workhorse))
-        optional 'metadata.sha256', type: String, desc: %q(sha256 checksum of metadata (generated by Workhorse))
+        optional :metadata, type: ::API::Validations::Types::WorkhorseFile, desc: %(The artifact metadata to store (generated by Multipart middleware))
       end
       post '/:id/artifacts' do
         not_allowed! unless Gitlab.config.artifacts.enabled
@@ -274,10 +267,9 @@ module API
         job = authenticate_job!
         forbidden!('Job is not running!') unless job.running?
 
-        artifacts = UploadedFile.from_params(params, :file, JobArtifactUploader.workhorse_local_upload_path)
-        metadata = UploadedFile.from_params(params, :metadata, JobArtifactUploader.workhorse_local_upload_path)
+        artifacts = params[:file]
+        metadata = params[:metadata]
 
-        bad_request!('Missing artifacts file!') unless artifacts
         file_too_large! unless artifacts.size < max_artifacts_size(job)
 
         result = Ci::CreateJobArtifactsService.new(job.project).execute(job, artifacts, params, metadata_file: metadata)

lib/gitlab/middleware/multipart.rb

@@ -107,6 +107,7 @@ module Gitlab
           [
             ::FileUploader.root,
             Gitlab.config.uploads.storage_path,
+            JobArtifactUploader.workhorse_upload_path,
             File.join(Rails.root, 'public/uploads/tmp')
           ]
         end
@@ -125,6 +126,8 @@ module Gitlab
         Handler.new(env, message).with_open_files do
           @app.call(env)
         end
+      rescue UploadedFile::InvalidPathError => e
+        [400, { 'Content-Type' => 'text/plain' }, e.message]
       end
     end
   end

spec/lib/gitlab/middleware/multipart_spec.rb

@@ -89,6 +89,17 @@ describe Gitlab::Middleware::Multipart do
     end
   end
 
+  it 'allows files in the job artifact upload path' do
+    with_tmp_dir('artifacts') do |dir, env|
+      expect(JobArtifactUploader).to receive(:workhorse_upload_path).and_return(File.join(dir, 'artifacts'))
+      expect(app).to receive(:call) do |env|
+        expect(get_params(env)['file']).to be_a(::UploadedFile)
+      end
+
+      middleware.call(env)
+    end
+  end
+
   it 'allows symlinks for uploads dir' do
     Tempfile.open('two-levels') do |tempfile|
       symlinked_dir = '/some/dir/uploads'

spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200204113225_schedule_recalculate_project_authorizations_third_run.rb')
+
+describe ScheduleRecalculateProjectAuthorizationsThirdRun do
+  let(:users_table) { table(:users) }
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 2)
+
+    1.upto(4) do |i|
+      users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1)
+    end
+  end
+
+  it 'schedules background migration' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(1, 2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(3, 4)
+      end
+    end
+  end
+end

spec/models/active_session_spec.rb

@@ -9,10 +9,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
-  let(:session) do
-    double(:session, { id: '6919a6f1bb119dd7396fadc38fd18d0d',
-                       '[]': {} })
-  end
+  let(:rack_session) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') }
+  let(:session) { instance_double(ActionDispatch::Request::Session, id: rack_session, '[]': {}) }
 
   let(:request) do
     double(:request, {
@@ -25,13 +23,13 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
   describe '#current?' do
     it 'returns true if the active session matches the current session' do
-      active_session = ActiveSession.new(session_id: '6919a6f1bb119dd7396fadc38fd18d0d')
+      active_session = ActiveSession.new(session_id: rack_session)
 
       expect(active_session.current?(session)).to be true
     end
 
     it 'returns false if the active session does not match the current session' do
-      active_session = ActiveSession.new(session_id: '59822c7d9fcdfa03725eff41782ad97d')
+      active_session = ActiveSession.new(session_id: Rack::Session::SessionId.new('59822c7d9fcdfa03725eff41782ad97d'))
 
       expect(active_session.current?(session)).to be false
     end
@@ -46,14 +44,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
   describe '#public_id' do
     it 'returns an encrypted, url-encoded session id' do
-      original_session_id = "!*'();:@&\n=+$,/?%abcd#123[4567]8"
+      original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8")
       active_session = ActiveSession.new(session_id: original_session_id)
-      encrypted_encoded_id = active_session.public_id
-
-      encrypted_id = CGI.unescape(encrypted_encoded_id)
+      encrypted_id = active_session.public_id
       derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id)
 
-      expect(original_session_id).to eq derived_session_id
+      expect(original_session_id.public_id).to eq derived_session_id
     end
   end
 
@@ -104,7 +100,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
   describe '.list_sessions' do
     it 'uses the ActiveSession lookup to return original sessions' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ _csrf_token: 'abcd' }))
+        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
+        redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' }))
 
         redis.sadd(
           "session:lookup:user:gitlab:#{user.id}",
@@ -127,17 +124,18 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         redis.sadd("session:lookup:user:gitlab:#{user.id}", session_ids)
       end
 
-      expect(ActiveSession.session_ids_for_user(user.id)).to eq(session_ids)
+      expect(ActiveSession.session_ids_for_user(user.id).map(&:to_s)).to eq(session_ids)
     end
   end
 
   describe '.sessions_from_ids' do
     it 'uses the ActiveSession lookup to return original sessions' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ _csrf_token: 'abcd' }))
+        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
+        redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' }))
       end
 
-      expect(ActiveSession.sessions_from_ids(['6919a6f1bb119dd7396fadc38fd18d0d'])).to eq [{ _csrf_token: 'abcd' }]
+      expect(ActiveSession.sessions_from_ids([rack_session])).to eq [{ _csrf_token: 'abcd' }]
     end
 
     it 'avoids a redis lookup for an empty array' do
@@ -152,11 +150,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
       redis = double(:redis)
       expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
 
-      sessions = %w[session-a session-b]
+      sessions = %w[session-a session-b session-c session-d]
       mget_responses = sessions.map { |session| [Marshal.dump(session)]}
-      expect(redis).to receive(:mget).twice.and_return(*mget_responses)
+      expect(redis).to receive(:mget).exactly(4).times.and_return(*mget_responses)
 
-      expect(ActiveSession.sessions_from_ids([1, 2])).to eql(sessions)
+      session_ids = [1, 2].map { |id| Rack::Session::SessionId.new(id.to_s) }
+      expect(ActiveSession.sessions_from_ids(session_ids).map(&:to_s)).to eql(sessions)
     end
   end
 
@@ -212,6 +211,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
   end
 
   describe '.destroy' do
+    it 'gracefully handles a nil session ID' do
+      expect(described_class).not_to receive(:destroy_sessions)
+
+      ActiveSession.destroy(user, nil)
+    end
+
     it 'removes the entry associated with the currently killed user session' do
       Gitlab::Redis::SharedState.with do |redis|
         redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
@@ -244,8 +249,9 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
     it 'removes the devise session' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
-        redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", '')
+        redis.set("session:user:gitlab:#{user.id}:#{rack_session.public_id}", '')
+        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
+        redis.set("session:gitlab:#{rack_session.private_id}", '')
       end
 
       ActiveSession.destroy(user, request.session.id)
@@ -322,7 +328,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
           (1..max_number_of_sessions_plus_two).each do |number|
             redis.set(
               "session:user:gitlab:#{user.id}:#{number}",
-              Marshal.dump(ActiveSession.new(session_id: "#{number}", updated_at: number.days.ago))
+              Marshal.dump(ActiveSession.new(session_id: number.to_s, updated_at: number.days.ago))
             )
             redis.sadd(
               "session:lookup:user:gitlab:#{user.id}",

spec/requests/api/runner_spec.rb

@@ -1422,8 +1422,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
 
     describe 'artifacts' do
       let(:job) { create(:ci_build, :pending, user: user, project: project, pipeline: pipeline, runner_id: runner.id) }
-      let(:jwt_token) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') }
-      let(:headers) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => jwt_token } }
+      let(:jwt) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') }
+      let(:headers) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => jwt } }
       let(:headers_with_token) { headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.token) }
       let(:file_upload) { fixture_file_upload('spec/fixtures/banana_sample.gif', 'image/gif') }
       let(:file_upload2) { fixture_file_upload('spec/fixtures/dk.png', 'image/gif') }
@@ -1703,12 +1703,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             it 'fails to post artifacts without GitLab-Workhorse' do
               post api("/jobs/#{job.id}/artifacts"), params: { token: job.token }, headers: {}
 
-              expect(response).to have_gitlab_http_status(:forbidden)
+              expect(response).to have_gitlab_http_status(:bad_request)
             end
           end
 
           context 'Is missing GitLab Workhorse token headers' do
-            let(:jwt_token) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') }
+            let(:jwt) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') }
 
             it 'fails to post artifacts without GitLab-Workhorse' do
               expect(Gitlab::ErrorTracking).to receive(:track_exception).once
@@ -1722,15 +1722,14 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           context 'when setting an expire date' do
             let(:default_artifacts_expire_in) {}
             let(:post_data) do
-              { 'file.path' => file_upload.path,
-                'file.name' => file_upload.original_filename,
-                'expire_in' => expire_in }
+              { file: file_upload,
+                expire_in: expire_in }
             end
 
             before do
               stub_application_setting(default_artifacts_expire_in: default_artifacts_expire_in)
 
-              post(api("/jobs/#{job.id}/artifacts"), params: post_data, headers: headers_with_token)
+              upload_artifacts(file_upload, headers_with_token, post_data)
             end
 
             context 'when an expire_in is given' do
@@ -1783,20 +1782,22 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             let(:stored_artifacts_size) { job.reload.artifacts_size }
             let(:stored_artifacts_sha256) { job.reload.job_artifacts_archive.file_sha256 }
             let(:stored_metadata_sha256) { job.reload.job_artifacts_metadata.file_sha256 }
+            let(:file_keys) { post_data.keys }
+            let(:send_rewritten_field) { true }
 
             before do
-              post(api("/jobs/#{job.id}/artifacts"), params: post_data, headers: headers_with_token)
+              workhorse_finalize_with_multiple_files(
+                api("/jobs/#{job.id}/artifacts"),
+                method: :post,
+                file_keys: file_keys,
+                params: post_data,
+                headers: headers_with_token,
+                send_rewritten_field: send_rewritten_field
+              )
             end
 
             context 'when posts data accelerated by workhorse is correct' do
-              let(:post_data) do
-                { 'file.path' => artifacts.path,
-                  'file.name' => artifacts.original_filename,
-                  'file.sha256' => artifacts_sha256,
-                  'metadata.path' => metadata.path,
-                  'metadata.name' => metadata.original_filename,
-                  'metadata.sha256' => metadata_sha256 }
-              end
+              let(:post_data) { { file: artifacts, metadata: metadata } }
 
               it 'stores artifacts and artifacts metadata' do
                 expect(response).to have_gitlab_http_status(:created)
@@ -1808,9 +1809,30 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
               end
             end
 
+            context 'with a malicious file.path param' do
+              let(:post_data) { {} }
+              let(:tmp_file) { Tempfile.new('crafted.file.path') }
+              let(:url) { "/jobs/#{job.id}/artifacts?file.path=#{tmp_file.path}" }
+
+              it 'rejects the request' do
+                expect(response).to have_gitlab_http_status(:bad_request)
+                expect(stored_artifacts_size).to be_nil
+              end
+            end
+
+            context 'when workhorse header is missing' do
+              let(:post_data) { { file: artifacts, metadata: metadata } }
+              let(:send_rewritten_field) { false }
+
+              it 'rejects the request' do
+                expect(response).to have_gitlab_http_status(:bad_request)
+                expect(stored_artifacts_size).to be_nil
+              end
+            end
+
             context 'when there is no artifacts file in post data' do
               let(:post_data) do
-                { 'metadata' => metadata }
+                { metadata: metadata }
               end
 
               it 'is expected to respond with bad request' do
@@ -2053,7 +2075,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             method: :post,
             file_key: :file,
             params: params.merge(file: file),
-            headers: headers
+            headers: headers,
+            send_rewritten_field: true
           )
         end
       end

spec/services/groups/destroy_service_spec.rb

@@ -133,4 +133,55 @@ describe Groups::DestroyService do
       end
     end
   end
+
+  describe 'authorization updates', :sidekiq_inline do
+    context 'shared groups' do
+      let!(:shared_group) { create(:group, :private) }
+      let!(:shared_group_child) { create(:group, :private, parent: shared_group) }
+
+      let!(:project) { create(:project, group: shared_group) }
+      let!(:project_child) { create(:project, group: shared_group_child) }
+
+      before do
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+        group.refresh_members_authorized_projects
+      end
+
+      it 'updates project authorization' do
+        expect(user.can?(:read_project, project)).to eq(true)
+        expect(user.can?(:read_project, project_child)).to eq(true)
+
+        destroy_group(group, user, false)
+
+        expect(user.can?(:read_project, project)).to eq(false)
+        expect(user.can?(:read_project, project_child)).to eq(false)
+      end
+    end
+
+    context 'shared groups in the same group hierarchy' do
+      let!(:subgroup) { create(:group, :private, parent: group) }
+      let!(:subgroup_user) { create(:user) }
+
+      before do
+        subgroup.add_user(subgroup_user, Gitlab::Access::MAINTAINER)
+
+        create(:group_group_link, shared_group: group, shared_with_group: subgroup)
+        subgroup.refresh_members_authorized_projects
+      end
+
+      context 'group is deleted' do
+        it 'updates project authorization' do
+          expect { destroy_group(group, user, false) }.to(
+            change { subgroup_user.can?(:read_project, project) }.from(true).to(false))
+        end
+      end
+
+      context 'subgroup is deleted' do
+        it 'updates project authorization' do
+          expect { destroy_group(subgroup, user, false) }.to(
+            change { subgroup_user.can?(:read_project, project) }.from(true).to(false))
+        end
+      end
+    end
+  end
 end

spec/support/helpers/workhorse_helpers.rb

@@ -33,22 +33,36 @@ module WorkhorseHelpers
   # workhorse_finalize will transform file_key inside params as if it was the finalize call of an inline object storage upload.
   # note that based on the content of the params it can simulate a disc acceleration or an object storage upload
   def workhorse_finalize(url, method: :post, file_key:, params:, headers: {}, send_rewritten_field: false)
-    workhorse_request_with_file(method, url,
-                                file_key: file_key,
-                                params: params,
-                                extra_headers: headers,
-                                send_rewritten_field: send_rewritten_field
+    workhorse_finalize_with_multiple_files(url, method: method, file_keys: file_key, params: params, headers: headers, send_rewritten_field: send_rewritten_field)
+  end
+
+  def workhorse_finalize_with_multiple_files(url, method: :post, file_keys:, params:, headers: {}, send_rewritten_field: false)
+    workhorse_request_with_multiple_files(method, url,
+                                          file_keys: file_keys,
+                                          params: params,
+                                          extra_headers: headers,
+                                          send_rewritten_field: send_rewritten_field
     )
   end
 
   def workhorse_request_with_file(method, url, file_key:, params:, env: {}, extra_headers: {}, send_rewritten_field:)
+    workhorse_request_with_multiple_files(method, url, file_keys: file_key, params: params, env: env, extra_headers: extra_headers, send_rewritten_field: send_rewritten_field)
+  end
+
+  def workhorse_request_with_multiple_files(method, url, file_keys:, params:, env: {}, extra_headers: {}, send_rewritten_field:)
     workhorse_params = params.dup
-    file = workhorse_params.delete(file_key)
 
-    workhorse_params = workhorse_disk_accelerated_file_params(file_key, file).merge(workhorse_params)
+    file_keys = Array(file_keys)
+    rewritten_fields = {}
+
+    file_keys.each do |key|
+      file = workhorse_params.delete(key)
+      rewritten_fields[key] = file.path if file
+      workhorse_params = workhorse_disk_accelerated_file_params(key, file).merge(workhorse_params)
+    end
 
     headers = if send_rewritten_field
-                workhorse_rewritten_fields_header(file_key => file.path)
+                workhorse_rewritten_fields_header(rewritten_fields)
               else
                 {}
               end
@@ -75,7 +89,11 @@ module WorkhorseHelpers
       "#{key}.name" => file.original_filename,
       "#{key}.size" => file.size
     }.tap do |params|
-      params["#{key}.path"] = file.path if file.path
+      if file.path
+        params["#{key}.path"] = file.path
+        params["#{key}.sha256"] = Digest::SHA256.file(file.path).hexdigest
+      end
+
       params["#{key}.remote_id"] = file.remote_id if file.respond_to?(:remote_id) && file.remote_id.present?
     end
   end

spec/support/rails/test_case_patch.rb

+# frozen_string_literal: true
+#
+# This file pulls in the changes in https://github.com/rails/rails/pull/38063
+# to fix controller specs updated with the latest Rack versions.
+#
+# This file should be removed after that change ships. It is not
+# present in Rails 6.0.2.2.
+module ActionController
+  class TestRequest < ActionDispatch::TestRequest #:nodoc:
+    def self.new_session
+      TestSessionPatched.new
+    end
+  end
+
+  # Methods #destroy and #load! are overridden to avoid calling methods on the
+  # @store object, which does not exist for the TestSession class.
+  class TestSessionPatched < Rack::Session::Abstract::PersistedSecure::SecureSessionHash #:nodoc:
+    DEFAULT_OPTIONS = Rack::Session::Abstract::Persisted::DEFAULT_OPTIONS
+
+    def initialize(session = {})
+      super(nil, nil)
+      @id = Rack::Session::SessionId.new(SecureRandom.hex(16))
+      @data = stringify_keys(session)
+      @loaded = true
+    end
+
+    def exists?
+      true
+    end
+
+    def keys
+      @data.keys
+    end
+
+    def values
+      @data.values
+    end
+
+    def destroy
+      clear
+    end
+
+    def fetch(key, *args, &block)
+      @data.fetch(key.to_s, *args, &block)
+    end
+
+    private
+
+    def load!
+      @id
+    end
+  end
+end