Merge remote-tracking branch 'dev/12-8-stable' into 12-8-stable

spec/services/auth/container_registry_authentication_service_spec.rb

@@ -769,6 +769,15 @@ describe Auth::ContainerRegistryAuthenticationService do
     context 'when deploy token has read_registry as a scope' do
       let(:current_user) { create(:deploy_token, projects: [project]) }
+      shared_examples 'able to login' do
+        context 'registry provides read_container_image authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [:read_container_image] }
+
+          it_behaves_like 'an authenticated'
+        end
+      end
+
       context 'for public project' do
         let(:project) { create(:project, :public) }
@@ -783,6 +792,8 @@ describe Auth::ContainerRegistryAuthenticationService do
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
       context 'for internal project' do
@@ -799,6 +810,8 @@ describe Auth::ContainerRegistryAuthenticationService do
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
       context 'for private project' do
@@ -815,18 +828,38 @@ describe Auth::ContainerRegistryAuthenticationService do
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
     end
     context 'when deploy token does not have read_registry scope' do
       let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) }
+      shared_examples 'unable to login' do
+        context 'registry provides no container authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [] }
+
+          it_behaves_like 'a forbidden'
+        end
+
+        context 'registry provides inapplicable container authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [:download_code] }
+
+          it_behaves_like 'a forbidden'
+        end
+      end
+
       context 'for public project' do
         let(:project) { create(:project, :public) }
         context 'when pulling' do
           it_behaves_like 'a pullable'
         end
+
+        it_behaves_like 'unable to login'
       end
       context 'for internal project' do
@@ -835,6 +868,8 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'when pulling' do
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'unable to login'
       end
       context 'for private project' do
@@ -843,6 +878,15 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'when pulling' do
           it_behaves_like 'an inaccessible'
         end
+
+        context 'when logging in' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [] }
+
+          it_behaves_like 'a forbidden'
+        end
+
+        it_behaves_like 'unable to login'
       end
     end

spec/services/groups/group_links/destroy_service_spec.rb

@@ -40,24 +40,11 @@ describe Groups::GroupLinks::DestroyService, '#execute' do
     end
     it 'updates project authorization once per group' do
-      expect(GroupGroupLink).to receive(:delete)
+      expect(GroupGroupLink).to receive(:delete).and_call_original
       expect(group).to receive(:refresh_members_authorized_projects).once
       expect(another_group).to receive(:refresh_members_authorized_projects).once
       subject.execute(links)
     end
-
-    it 'rolls back changes when error happens' do
-      group.add_developer(user)
-
-      expect(group).to receive(:refresh_members_authorized_projects).once.and_call_original
-      expect(another_group).to(
-        receive(:refresh_members_authorized_projects).and_raise('boom'))
-
-      expect { subject.execute(links) }.to raise_error('boom')
-
-      expect(GroupGroupLink.count).to eq(links.length)
-      expect(Ability.allowed?(user, :read_project, project)).to be_truthy
-    end
   end
 end

spec/services/groups/group_links/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Groups::GroupLinks::UpdateService, '#execute' do
+  let(:user) { create(:user) }
+
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:shared_group) { create(:group, :private) }
+  let_it_be(:project) { create(:project, group: shared_group) }
+  let(:group_member) { create(:user) }
+  let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) }
+
+  let(:expiry_date) { 1.month.from_now.to_date }
+  let(:group_link_params) do
+    { group_access: Gitlab::Access::GUEST,
+      expires_at: expiry_date }
+  end
+
+  subject { described_class.new(link).execute(group_link_params) }
+
+  before do
+    group.add_developer(group_member)
+  end
+
+  it 'updates existing link' do
+    expect(link.group_access).to eq(Gitlab::Access::DEVELOPER)
+    expect(link.expires_at).to be_nil
+
+    subject
+
+    link.reload
+
+    expect(link.group_access).to eq(Gitlab::Access::GUEST)
+    expect(link.expires_at).to eq(expiry_date)
+  end
+
+  it 'updates project permissions' do
+    expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false)
+  end
+
+  it 'executes UserProjectAccessChangedService' do
+    expect_next_instance_of(UserProjectAccessChangedService) do |service|
+      expect(service).to receive(:execute)
+    end
+
+    subject
+  end
+
+  context 'with only param not requiring authorization refresh' do
+    let(:group_link_params) { { expires_at: Date.tomorrow } }
+
+    it 'does not execute UserProjectAccessChangedService' do
+      expect(UserProjectAccessChangedService).not_to receive(:new)
+
+      subject
+    end
+  end
+end

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -134,6 +134,21 @@ describe Projects::LfsPointers::LfsDownloadService do
       end
     end
+    context 'when an lfs object with the same oid already exists' do
+      let!(:existing_lfs_object) { create(:lfs_object, oid: oid) }
+
+      before do
+        stub_full_request(download_link).to_return(body: lfs_content)
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'does not update the file attached to the existing LfsObject' do
+        expect { subject.execute }
+          .not_to change { existing_lfs_object.reload.file.file.file }
+      end
+    end
+
     context 'when credentials present' do
       let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" }
       let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) }
@@ -211,17 +226,5 @@ describe Projects::LfsPointers::LfsDownloadService do
         subject.execute
       end
     end
-
-    context 'when an lfs object with the same oid already exists' do
-      before do
-        create(:lfs_object, oid: oid)
-      end
-
-      it 'does not download the file' do
-        expect(subject).not_to receive(:download_lfs_file!)
-
-        subject.execute
-      end
-    end
   end
 end

spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb

@@ -24,7 +24,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
   describe '#execute' do
     context 'when no lfs pointer is linked' do
       before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([])
         allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
         expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original
       end
@@ -35,12 +34,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
         subject.execute
       end
-      it 'links existent lfs objects to the project' do
-        expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute)
-
-        subject.execute
-      end
-
       it 'retrieves the download links of non existent objects' do
         expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids)
@@ -48,32 +41,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
       end
     end
-    context 'when some lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
-      end
-
-      it 'retrieves the download links of non existent objects' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids)
-
-        subject.execute
-      end
-    end
-
-    context 'when all lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute)
-      end
-
-      it 'retrieves no download links' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original
-
-        expect(subject.execute).to be_empty
-      end
-    end
-
     context 'when lfsconfig file exists' do
       before do
         allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n")

spec/support/shared_contexts/features/error_tracking_shared_context.rb

@@ -13,7 +13,7 @@ shared_context 'sentry error tracking context feature' do
   let(:issue_id) { issue_response['id'] }
   let(:issue_seen) { 1.year.ago.utc }
   let(:formatted_issue_seen) { issue_seen.strftime("%Y-%m-%d %-l:%M:%S%p %Z") }
-  let(:date_received) { 1.month.ago.utc }
+  let(:date_received) { 32.days.ago.utc }
   before do
     request_headers = { 'Authorization' => 'Bearer access_token_123', 'Content-Type' => 'application/json' }

spec/support/shared_contexts/policies/group_policy_shared_context.rb

@@ -7,6 +7,7 @@ RSpec.shared_context 'GroupPolicy context' do
   let_it_be(:maintainer) { create(:user) }
   let_it_be(:owner) { create(:user) }
   let_it_be(:admin) { create(:admin) }
+  let_it_be(:non_group_member) { create(:user) }
   let_it_be(:group, refind: true) { create(:group, :private, :owner_subgroup_creation_only) }
   let(:guest_permissions) do

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -69,13 +69,39 @@ RSpec.shared_examples 'handle uploads' do
   end
   describe "GET #show" do
+    let(:filename) { "rails_sample.jpg" }
+
+    let(:upload_service) do
+      UploadService.new(model, jpg, uploader_class).execute
+    end
+
     let(:show_upload) do
-      get :show, params: params.merge(secret: secret, filename: "rails_sample.jpg")
+      get :show, params: params.merge(secret: secret, filename: filename)
     end
     before do
       allow(FileUploader).to receive(:generate_secret).and_return(secret)
-      UploadService.new(model, jpg, uploader_class).execute
+      upload_service
+    end
+
+    context 'when the secret is invalid' do
+      let(:secret) { "../../../../../../../../" }
+      let(:filename) { "Gemfile.lock" }
+      let(:upload_service) { nil }
+
+      it 'responds with status 404' do
+        show_upload
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+
+      it 'is a working exploit without the validation' do
+        allow_any_instance_of(FileUploader).to receive(:secret) { secret }
+
+        show_upload
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
     end
     context 'when accessing a specific upload via different model' do

spec/uploaders/file_mover_spec.rb

@@ -7,7 +7,7 @@ describe FileMover do
   let(:user) { create(:user) }
   let(:filename) { 'banana_sample.gif' }
-  let(:secret) { 'secret55' }
+  let(:secret) { SecureRandom.hex }
   let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) }
   let(:temp_description) do
@@ -47,8 +47,8 @@ describe FileMover do
           subject
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
+            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ")
         end
         it 'updates existing upload record' do
@@ -75,8 +75,8 @@ describe FileMover do
           subject
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
+            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ")
         end
         it 'does not change the upload record' do
@@ -101,8 +101,8 @@ describe FileMover do
           subject
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
+            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ")
         end
         it 'creates new target upload record an delete the old upload' do
@@ -121,8 +121,8 @@ describe FileMover do
           subject
           expect(snippet.reload.description)
-            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
-                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
+            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ")
         end
         it 'does not change the upload record' do
@@ -138,12 +138,8 @@ describe FileMover do
       let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') }
       it 'does not trigger move if path is outside designated directory' do
-        stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp")
         expect(FileUtils).not_to receive(:move)
-
-        subject
-
-        expect(snippet.reload.description).to eq(temp_description)
+        expect { subject }.to raise_error(FileUploader::InvalidSecret)
       end
     end

spec/uploaders/file_uploader_spec.rb

@@ -6,7 +6,8 @@ describe FileUploader do
   let(:group) { create(:group, name: 'awesome') }
   let(:project) { create(:project, :legacy_storage, namespace: group, name: 'project') }
   let(:uploader) { described_class.new(project, :avatar) }
-  let(:upload) { double(model: project, path: 'secret/foo.jpg') }
+  let(:upload) { double(model: project, path: "#{secret}/foo.jpg") }
+  let(:secret) { "55dc16aa0edd05693fd98b5051e83321" } # this would be nicer as SecureRandom.hex, but the shared_examples breaks
   subject { uploader }
@@ -14,7 +15,7 @@ describe FileUploader do
     include_examples 'builds correct paths',
                      store_dir: %r{awesome/project/\h+},
                      upload_path: %r{\h+/<filename>},
-                     absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg}
+                     absolute_path: %r{#{described_class.root}/awesome/project/55dc16aa0edd05693fd98b5051e83321/foo.jpg}
   end
   context 'legacy storage' do
@@ -51,11 +52,11 @@ describe FileUploader do
   end
   describe 'initialize' do
-    let(:uploader) { described_class.new(double, secret: 'secret') }
+    let(:uploader) { described_class.new(double, secret: secret) }
     it 'accepts a secret parameter' do
       expect(described_class).not_to receive(:generate_secret)
-      expect(uploader.secret).to eq('secret')
+      expect(uploader.secret).to eq(secret)
     end
   end
@@ -154,8 +155,39 @@ describe FileUploader do
   describe '#secret' do
     it 'generates a secret if none is provided' do
-      expect(described_class).to receive(:generate_secret).and_return('secret')
-      expect(uploader.secret).to eq('secret')
+      expect(described_class).to receive(:generate_secret).and_return(secret)
+      expect(uploader.secret).to eq(secret)
+      expect(uploader.secret.size).to eq(32)
+    end
+
+    context "validation" do
+      before do
+        uploader.instance_variable_set(:@secret, secret)
+      end
+
+      context "32-byte hexadecimal" do
+        let(:secret) { SecureRandom.hex }
+
+        it "returns the secret" do
+          expect(uploader.secret).to eq(secret)
+        end
+      end
+
+      context "10-byte hexadecimal" do
+        let(:secret) { SecureRandom.hex(10) }
+
+        it "returns the secret" do
+          expect(uploader.secret).to eq(secret)
+        end
+      end
+
+      context "invalid secret supplied" do
+        let(:secret) { "%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2Fgrafana%2Fconf%2F" }
+
+        it "raises an exception" do
+          expect { uploader.secret }.to raise_error(described_class::InvalidSecret)
+        end
+      end
     end
   end

spec/uploaders/personal_file_uploader_spec.rb

@@ -6,12 +6,13 @@ describe PersonalFileUploader do
   let(:model) { create(:personal_snippet) }
   let(:uploader) { described_class.new(model) }
   let(:upload) { create(:upload, :personal_snippet_upload) }
+  let(:secret) { SecureRandom.hex }
   subject { uploader }
   shared_examples '#base_dir' do
     before do
-      subject.instance_variable_set(:@secret, 'secret')
+      subject.instance_variable_set(:@secret, secret)
     end
     it 'is prefixed with uploads/-/system' do
@@ -23,12 +24,12 @@ describe PersonalFileUploader do
   shared_examples '#to_h' do
     before do
-      subject.instance_variable_set(:@secret, 'secret')
+      subject.instance_variable_set(:@secret, secret)
     end
     it 'is correct' do
       allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
-      expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name"
+      expected_url = "/uploads/-/system/personal_snippet/#{model.id}/#{secret}/file_name"
       expect(uploader.to_h).to eq(
         alt: 'file_name',

spec/validators/addressable_url_validator_spec.rb

@@ -5,6 +5,9 @@ require 'spec_helper'
 describe AddressableUrlValidator do
   let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
+  let(:validator) { described_class.new(validator_options.reverse_merge(attributes: [:link_url])) }
+  let(:validator_options) { {} }
+
   subject { validator.validate(badge) }
   include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes]
@@ -114,6 +117,19 @@ describe AddressableUrlValidator do
     end
   end
+  context 'when blocked_message is set' do
+    let(:message) { 'is not allowed due to: %{exception_message}' }
+    let(:validator_options) { { blocked_message: message } }
+
+    it 'blocks url with provided error message' do
+      badge.link_url = 'javascript:alert(window.opener.document.location)'
+
+      subject
+
+      expect(badge.errors.first[1]).to eq 'is not allowed due to: Only allowed schemes are http, https'
+    end
+  end
+
   context 'when allow_nil is set to true' do
     let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) }