diff options
author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2020-04-15 00:21:55 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2020-04-15 00:21:55 +0000 |
commit | 1c972f15be039a69e897f7e0a78606ac9402b2c1 (patch) | |
tree | a0565af647961388584d0464f2fb2f91e87816e8 | |
parent | 4d968442247abb94832fa6136ea90869699370e8 (diff) | |
parent | f4e077c43103a585f64b116b004ebb3118de0b30 (diff) | |
download | gitlab-ce-1c972f15be039a69e897f7e0a78606ac9402b2c1.tar.gz |
Merge remote-tracking branch 'dev/12-8-stable' into 12-8-stable
-rw-r--r-- | CHANGELOG.md | 9 | ||||
-rw-r--r-- | GITALY_SERVER_VERSION | 2 | ||||
-rw-r--r-- | GITLAB_WORKHORSE_VERSION | 2 | ||||
-rw-r--r-- | Gemfile | 2 | ||||
-rw-r--r-- | Gemfile.lock | 16 | ||||
-rw-r--r-- | VERSION | 2 | ||||
-rw-r--r-- | app/models/active_session.rb | 99 | ||||
-rw-r--r-- | app/services/groups/destroy_service.rb | 8 | ||||
-rw-r--r-- | db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb | 28 | ||||
-rw-r--r-- | lib/api/runner.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/middleware/multipart.rb | 3 | ||||
-rw-r--r-- | spec/lib/gitlab/middleware/multipart_spec.rb | 11 | ||||
-rw-r--r-- | spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb | 28 | ||||
-rw-r--r-- | spec/models/active_session_spec.rb | 48 | ||||
-rw-r--r-- | spec/requests/api/runner_spec.rb | 61 | ||||
-rw-r--r-- | spec/services/groups/destroy_service_spec.rb | 51 | ||||
-rw-r--r-- | spec/support/helpers/workhorse_helpers.rb | 38 | ||||
-rw-r--r-- | spec/support/rails/test_case_patch.rb | 53 | ||||
-rwxr-xr-x[-rw-r--r--] | vendor/gitignore/C++.gitignore | 0 | ||||
-rwxr-xr-x[-rw-r--r--] | vendor/gitignore/Java.gitignore | 0 |
20 files changed, 373 insertions, 104 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f9b53846b9..ff03f9761f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 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.8 (2020-03-26) ### Security (17 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index aef81b964a7..a4fef53f7fc 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -12.8.8 +12.8.9 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index a4ea2549bb6..d504c40781a 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.21.1 +8.21.2 @@ -163,7 +163,7 @@ gem 'diffy', '~> 3.1.0' 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' diff --git a/Gemfile.lock b/Gemfile.lock index 9735b9be935..5a8045a1667 100644 --- a/Gemfile.lock +++ b/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) @@ -783,7 +783,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) @@ -854,17 +854,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) @@ -1325,7 +1325,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) @@ -1 +1 @@ -12.8.8 +12.8.9 diff --git a/app/models/active_session.rb b/app/models/active_session.rb index f37da1b7f59..050155398ab 100644 --- a/app/models/active_session.rb +++ b/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 diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index c9c6b54a791..9437eb9eede 100644 --- a/app/services/groups/destroy_service.rb +++ b/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 diff --git a/db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb b/db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb new file mode 100644 index 00000000000..47b22b4800a --- /dev/null +++ b/db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb @@ -0,0 +1,28 @@ +# 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 diff --git a/lib/api/runner.rb b/lib/api/runner.rb index e1c79aa8efe..a921182fa72 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -254,21 +254,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 @@ -277,10 +270,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) if Ci::CreateJobArtifactsService.new.execute(job, artifacts, params, metadata_file: metadata) diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb index cb4213d23a4..c82c05e7319 100644 --- a/lib/gitlab/middleware/multipart.rb +++ b/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 diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb index 1d3ad3afb56..ec153e25d44 100644 --- a/spec/lib/gitlab/middleware/multipart_spec.rb +++ b/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' diff --git a/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb b/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb new file mode 100644 index 00000000000..a5f1c231d08 --- /dev/null +++ b/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20200204113225_schedule_recalculate_project_authorizations_third_run.rb') + +describe ScheduleRecalculateProjectAuthorizationsThirdRun, :migration 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 diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb index bff3ac313c4..d43c64f36f6 100644 --- a/spec/models/active_session_spec.rb +++ b/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}", diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 9d01a44916c..2d25a35bc50 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1435,8 +1435,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') } @@ -1716,12 +1716,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(403) + 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 @@ -1735,15 +1735,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 @@ -1796,20 +1795,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(201) @@ -1821,9 +1822,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 @@ -2008,7 +2030,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 diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index a45c7cdffa6..bf639153b99 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/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 diff --git a/spec/support/helpers/workhorse_helpers.rb b/spec/support/helpers/workhorse_helpers.rb index 0de8b382403..53b36b3dd45 100644 --- a/spec/support/helpers/workhorse_helpers.rb +++ b/spec/support/helpers/workhorse_helpers.rb @@ -32,23 +32,37 @@ 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: {}) - workhorse_request_with_file(method, url, - file_key: file_key, - params: params, - extra_headers: headers, - send_rewritten_field: false + def workhorse_finalize(url, method: :post, file_key:, params:, headers: {}, send_rewritten_field: false) + 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 diff --git a/spec/support/rails/test_case_patch.rb b/spec/support/rails/test_case_patch.rb new file mode 100644 index 00000000000..161e1ef2a4c --- /dev/null +++ b/spec/support/rails/test_case_patch.rb @@ -0,0 +1,53 @@ +# 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 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 |