Merge branch 'security-fix-lfs-import-project-ssrf-forgery' into 'master'

[master] LFS object forgery in project import

Closes #2784

See merge request gitlab/gitlabhq!2719

app/models/lfs_download_object.rb

+# frozen_string_literal: true
+
+class LfsDownloadObject
+  include ActiveModel::Validations
+
+  attr_accessor :oid, :size, :link
+  delegate :sanitized_url, :credentials, to: :sanitized_uri
+
+  validates :oid, format: { with: /\A\h{64}\z/ }
+  validates :size, numericality: { greater_than_or_equal_to: 0 }
+  validates :link, public_url: { protocols: %w(http https) }
+
+  def initialize(oid:, size:, link:)
+    @oid = oid
+    @size = size
+    @link = link
+  end
+
+  def sanitized_uri
+    @sanitized_uri ||= Gitlab::UrlSanitizer.new(link)
+  end
+end

app/services/projects/import_service.rb

@@ -86,11 +86,11 @@ module Projects
 
       return unless project.lfs_enabled?
 
-      oids_to_download = Projects::LfsPointers::LfsImportService.new(project).execute
-      download_service = Projects::LfsPointers::LfsDownloadService.new(project)
+      lfs_objects_to_download = Projects::LfsPointers::LfsImportService.new(project).execute
 
-      oids_to_download.each do |oid, link|
-        download_service.execute(oid, link)
+      lfs_objects_to_download.each do |lfs_download_object|
+        Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object)
+                                                 .execute
       end
     rescue => e
       # Right now, to avoid aborting the importing process, we silently fail

app/services/projects/lfs_pointers/lfs_download_link_list_service.rb

@@ -41,16 +41,17 @@ module Projects
       end
 
       def parse_response_links(objects_response)
-        objects_response.each_with_object({}) do |entry, link_list|
+        objects_response.each_with_object([]) do |entry, link_list|
           begin
-            oid = entry['oid']
             link = entry.dig('actions', DOWNLOAD_ACTION, 'href')
 
             raise DownloadLinkNotFound unless link
 
-            link_list[oid] = add_credentials(link)
-          rescue DownloadLinkNotFound, URI::InvalidURIError
-            Rails.logger.error("Link for Lfs Object with oid #{oid} not found or invalid.")
+            link_list << LfsDownloadObject.new(oid: entry['oid'],
+                                               size: entry['size'],
+                                               link: add_credentials(link))
+          rescue DownloadLinkNotFound, Addressable::URI::InvalidURIError
+            log_error("Link for Lfs Object with oid #{entry['oid']} not found or invalid.")
           end
         end
       end
@@ -70,7 +71,7 @@ module Projects
       end
 
       def add_credentials(link)
-        uri = URI.parse(link)
+        uri = Addressable::URI.parse(link)
 
         if should_add_credentials?(uri)
           uri.user = remote_uri.user

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -4,68 +4,93 @@
 module Projects
   module LfsPointers
     class LfsDownloadService < BaseService
-      VALID_PROTOCOLS = %w[http https].freeze
+      SizeError = Class.new(StandardError)
+      OidError = Class.new(StandardError)
 
-      # rubocop: disable CodeReuse/ActiveRecord
-      def execute(oid, url)
-        return unless project&.lfs_enabled? && oid.present? && url.present?
+      attr_reader :lfs_download_object
+      delegate :oid, :size, :credentials, :sanitized_url, to: :lfs_download_object, prefix: :lfs
 
-        return if LfsObject.exists?(oid: oid)
+      def initialize(project, lfs_download_object)
+        super(project)
 
-        sanitized_uri = sanitize_url!(url)
+        @lfs_download_object = lfs_download_object
+      end
 
-        with_tmp_file(oid) do |file|
-          download_and_save_file(file, sanitized_uri)
-          lfs_object = LfsObject.new(oid: oid, size: file.size, file: file)
+      # rubocop: disable CodeReuse/ActiveRecord
+      def execute
+        return unless project&.lfs_enabled? && lfs_download_object
+        return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid?
+        return if LfsObject.exists?(oid: lfs_oid)
 
-          project.all_lfs_objects << lfs_object
+        wrap_download_errors do
+          download_lfs_file!
         end
-      rescue Gitlab::UrlBlocker::BlockedUrlError => e
-        Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}")
-      rescue StandardError => e
-        Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}")
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       private
 
-      def sanitize_url!(url)
-        Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri|
-          # Just validate that HTTP/HTTPS protocols are used. The
-          # subsequent Gitlab::HTTP.get call will do network checks
-          # based on the settings.
-          Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url,
-                                       protocols: VALID_PROTOCOLS)
+      def wrap_download_errors(&block)
+        yield
+      rescue SizeError, OidError, StandardError => e
+        error("LFS file with oid #{lfs_oid} could't be downloaded from #{lfs_sanitized_url}: #{e.message}")
+      end
+
+      def download_lfs_file!
+        with_tmp_file do |tmp_file|
+          download_and_save_file!(tmp_file)
+          project.all_lfs_objects << LfsObject.new(oid: lfs_oid,
+                                                   size: lfs_size,
+                                                   file: tmp_file)
+
+          success
         end
       end
 
-      def download_and_save_file(file, sanitized_uri)
-        response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment|
+      def download_and_save_file!(file)
+        digester = Digest::SHA256.new
+        response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment|
+          digester << fragment
           file.write(fragment)
+
+          raise_size_error! if file.size > lfs_size
         end
 
         raise StandardError, "Received error code #{response.code}" unless response.success?
-      end
 
-      def headers(sanitized_uri)
-        query_options.tap do |headers|
-          credentials = sanitized_uri.credentials
+        raise_size_error! if file.size != lfs_size
+        raise_oid_error! if digester.hexdigest != lfs_oid
+      end
 
-          if credentials[:user].present? || credentials[:password].present?
+      def download_headers
+        { stream_body: true }.tap do |headers|
+          if lfs_credentials[:user].present? || lfs_credentials[:password].present?
             # Using authentication headers in the request
-            headers[:http_basic_authentication] = [credentials[:user], credentials[:password]]
+            headers[:basic_auth] = { username: lfs_credentials[:user], password: lfs_credentials[:password] }
           end
         end
       end
 
-      def query_options
-        { stream_body: true }
-      end
-
-      def with_tmp_file(oid)
+      def with_tmp_file
         create_tmp_storage_dir
 
-        File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file }
+        File.open(tmp_filename, 'wb') do |file|
+          begin
+            yield file
+          rescue StandardError => e
+            # If the lfs file is successfully downloaded it will be removed
+            # when it is added to the project's lfs files.
+            # Nevertheless if any excetion raises the file would remain
+            # in the file system. Here we ensure to remove it
+            File.unlink(file) if File.exist?(file)
+
+            raise e
+          end
+        end
+      end
+
+      def tmp_filename
+        File.join(tmp_storage_dir, lfs_oid)
       end
 
       def create_tmp_storage_dir
@@ -79,6 +104,20 @@ module Projects
       def storage_dir
         @storage_dir ||= Gitlab.config.lfs.storage_path
       end
+
+      def raise_size_error!
+        raise SizeError, 'Size mistmatch'
+      end
+
+      def raise_oid_error!
+        raise OidError, 'Oid mismatch'
+      end
+
+      def error(message, http_status = nil)
+        log_error(message)
+
+        super
+      end
     end
   end
 end

changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml

+---
+title: Add more LFS validations to prevent forgery
+merge_request:
+author:
+type: security

lib/gitlab/github_import/importer/lfs_object_importer.rb

@@ -13,10 +13,12 @@ module Gitlab
           @project = project
         end
 
+        def lfs_download_object
+          LfsDownloadObject.new(oid: lfs_object.oid, size: lfs_object.size, link: lfs_object.link)
+        end
+
         def execute
-          Projects::LfsPointers::LfsDownloadService
-            .new(project)
-            .execute(lfs_object.oid, lfs_object.download_link)
+          Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object).execute
         end
       end
     end

lib/gitlab/github_import/representation/lfs_object.rb

@@ -9,11 +9,11 @@ module Gitlab
 
         attr_reader :attributes
 
-        expose_attribute :oid, :download_link
+        expose_attribute :oid, :link, :size
 
         # Builds a lfs_object
         def self.from_api_response(lfs_object)
-          new({ oid: lfs_object[0], download_link: lfs_object[1] })
+          new({ oid: lfs_object.oid, link: lfs_object.link, size: lfs_object.size })
         end
 
         # Builds a new lfs_object using a Hash that was built from a JSON payload.

spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb

@@ -2,20 +2,26 @@ require 'spec_helper'
 
 describe Gitlab::GithubImport::Importer::LfsObjectImporter do
   let(:project) { create(:project) }
-  let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" }
-
-  let(:github_lfs_object) do
-    Gitlab::GithubImport::Representation::LfsObject.new(
-      oid: 'oid', download_link: download_link
-    )
+  let(:lfs_attributes) do
+    {
+      oid: 'oid',
+      size: 1,
+      link: 'http://www.gitlab.com/lfs_objects/oid'
+    }
   end
 
+  let(:lfs_download_object) { LfsDownloadObject.new(lfs_attributes) }
+  let(:github_lfs_object) { Gitlab::GithubImport::Representation::LfsObject.new(lfs_attributes) }
+
   let(:importer) { described_class.new(github_lfs_object, project, nil) }
 
   describe '#execute' do
     it 'calls the LfsDownloadService with the lfs object attributes' do
-      expect_any_instance_of(Projects::LfsPointers::LfsDownloadService)
-        .to receive(:execute).with('oid', download_link)
+      allow(importer).to receive(:lfs_download_object).and_return(lfs_download_object)
+
+      service = double
+      expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).with(project, lfs_download_object).and_return(service)
+      expect(service).to receive(:execute)
 
       importer.execute
     end

spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb

@@ -5,7 +5,15 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do
   let(:client) { double(:client) }
   let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" }
 
-  let(:github_lfs_object) { ['oid', download_link] }
+  let(:lfs_attributes) do
+    {
+      oid: 'oid',
+      size: 1,
+      link: 'http://www.gitlab.com/lfs_objects/oid'
+    }
+  end
+
+  let(:lfs_download_object) { LfsDownloadObject.new(lfs_attributes) }
 
   describe '#parallel?' do
     it 'returns true when running in parallel mode' do
@@ -48,7 +56,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do
 
       allow(importer)
         .to receive(:each_object_to_import)
-        .and_yield(['oid', download_link])
+        .and_yield(lfs_download_object)
 
       expect(Gitlab::GithubImport::Importer::LfsObjectImporter)
         .to receive(:new)
@@ -71,7 +79,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do
 
       allow(importer)
         .to receive(:each_object_to_import)
-        .and_yield(github_lfs_object)
+        .and_yield(lfs_download_object)
 
       expect(Gitlab::GithubImport::ImportLfsObjectWorker)
         .to receive(:perform_async)

spec/models/lfs_download_object_spec.rb

+require 'rails_helper'
+
+describe LfsDownloadObject do
+  let(:oid) { 'cd293be6cea034bd45a0352775a219ef5dc7825ce55d1f7dae9762d80ce64411' }
+  let(:link) { 'http://www.example.com' }
+  let(:size) { 1 }
+
+  subject { described_class.new(oid: oid, size: size, link: link) }
+
+  describe 'validations' do
+    it { is_expected.to validate_numericality_of(:size).is_greater_than_or_equal_to(0) }
+
+    context 'oid attribute' do
+      it 'must be 64 characters long' do
+        aggregate_failures do
+          expect(described_class.new(oid: 'a' * 63, size: size, link: link)).to be_invalid
+          expect(described_class.new(oid: 'a' * 65, size: size, link: link)).to be_invalid
+          expect(described_class.new(oid: 'a' * 64, size: size, link: link)).to be_valid
+        end
+      end
+
+      it 'must contain only hexadecimal characters' do
+        aggregate_failures do
+          expect(subject).to be_valid
+          expect(described_class.new(oid: 'g' * 64, size: size, link: link)).to be_invalid
+        end
+      end
+    end
+
+    context 'link attribute' do
+      it 'only http and https protocols are valid' do
+        aggregate_failures do
+          expect(described_class.new(oid: oid, size: size, link: 'http://www.example.com')).to be_valid
+          expect(described_class.new(oid: oid, size: size, link: 'https://www.example.com')).to be_valid
+          expect(described_class.new(oid: oid, size: size, link: 'ftp://www.example.com')).to be_invalid
+          expect(described_class.new(oid: oid, size: size, link: 'ssh://www.example.com')).to be_invalid
+          expect(described_class.new(oid: oid, size: size, link: 'git://www.example.com')).to be_invalid
+        end
+      end
+
+      it 'cannot be empty' do
+        expect(described_class.new(oid: oid, size: size, link: '')).not_to be_valid
+      end
+
+      context 'when localhost or local network addresses' do
+        subject { described_class.new(oid: oid, size: size, link: 'http://192.168.1.1') }
+
+        before do
+          allow(ApplicationSetting)
+            .to receive(:current)
+              .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: setting))
+        end
+
+        context 'are allowed' do
+          let(:setting) { true }
+
+          it { expect(subject).to be_valid }
+        end
+
+        context 'are not allowed' do
+          let(:setting) { false }
+
+          it { expect(subject).to be_invalid }
+        end
+      end
+    end
+  end
+end

spec/services/projects/import_service_spec.rb

@@ -152,8 +152,11 @@ describe Projects::ImportService do
 
           it 'downloads lfs objects if lfs_enabled is enabled for project' do
             allow(project).to receive(:lfs_enabled?).and_return(true)
+
+            service = double
             expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links)
-            expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).to receive(:execute).twice
+            expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice
+            expect(service).to receive(:execute).twice
 
             subject.execute
           end
@@ -211,8 +214,10 @@ describe Projects::ImportService do
         it 'does not have a custom repository importer downloads lfs objects' do
           allow(Gitlab::GithubImport::ParallelImporter).to receive(:imports_repository?).and_return(false)
 
+          service = double
           expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links)
-          expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).to receive(:execute)
+          expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice
+          expect(service).to receive(:execute).twice
 
           subject.execute
         end

spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb

@@ -37,8 +37,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
 
   describe '#execute' do
     it 'retrieves each download link of every non existent lfs object' do
-      subject.execute(new_oids).each do |oid, link|
-        expect(link).to eq "#{import_url}/gitlab-lfs/objects/#{oid}"
+      subject.execute(new_oids).each do |lfs_download_object|
+        expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}"
       end
     end
 
@@ -50,8 +50,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
           it 'adds credentials to the download_link' do
             result = subject.execute(new_oids)
 
-            result.each do |oid, link|
-              expect(link.starts_with?('http://user:password@')).to be_truthy
+            result.each do |lfs_download_object|
+              expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_truthy
             end
           end
         end
@@ -60,8 +60,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
           it 'does not add any credentials' do
             result = subject.execute(new_oids)
 
-            result.each do |oid, link|
-              expect(link.starts_with?('http://user:password@')).to be_falsey
+            result.each do |lfs_download_object|
+              expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey
             end
           end
         end
@@ -74,8 +74,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
         it 'downloads without any credentials' do
           result = subject.execute(new_oids)
 
-          result.each do |oid, link|
-            expect(link.starts_with?('http://user:password@')).to be_falsey
+          result.each do |lfs_download_object|
+            expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey
           end
         end
       end
@@ -92,7 +92,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
 
   describe '#parse_response_links' do
     it 'does not add oid entry if href not found' do
-      expect(Rails.logger).to receive(:error).with("Link for Lfs Object with oid whatever not found or invalid.")
+      expect(subject).to receive(:log_error).with("Link for Lfs Object with oid whatever not found or invalid.")
 
       result = subject.send(:parse_response_links, invalid_object_response)
 

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -2,68 +2,156 @@ require 'spec_helper'
 
 describe Projects::LfsPointers::LfsDownloadService do
   let(:project) { create(:project) }
-  let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' }
-  let(:download_link) { "http://gitlab.com/#{oid}" }
   let(:lfs_content) { SecureRandom.random_bytes(10) }
+  let(:oid) { Digest::SHA256.hexdigest(lfs_content) }
+  let(:download_link) { "http://gitlab.com/#{oid}" }
+  let(:size) { lfs_content.size }
+  let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link) }
+  let(:local_request_setting) { false }
 
-  subject { described_class.new(project) }
+  subject { described_class.new(project, lfs_object) }
 
   before do
+    ApplicationSetting.create_from_defaults
+
+    stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting)
     allow(project).to receive(:lfs_enabled?).and_return(true)
-    WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+  end
+
+  shared_examples 'lfs temporal file is removed' do
+    it do
+      subject.execute
 
-    allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
+      expect(File.exist?(subject.send(:tmp_filename))).to be false
+    end
+  end
+
+  shared_examples 'no lfs object is created' do
+    it do
+      expect { subject.execute }.not_to change { LfsObject.count }
+    end
+
+    it 'returns error result' do
+      expect(subject.execute[:status]).to eq :error
+    end
+
+    it 'an error is logged' do
+      expect(subject).to receive(:log_error)
+
+      subject.execute
+    end
+
+    it_behaves_like 'lfs temporal file is removed'
+  end
+
+  shared_examples 'lfs object is created' do
+    it do
+      expect(subject).to receive(:download_and_save_file!).and_call_original
+
+      expect { subject.execute }.to change { LfsObject.count }.by(1)
+    end
+
+    it 'returns success result' do
+      expect(subject.execute[:status]).to eq :success
+    end
+
+    it_behaves_like 'lfs temporal file is removed'
   end
 
   describe '#execute' do
     context 'when file download succeeds' do
-      it 'a new lfs object is created' do
-        expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1)
+      before do
+        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
       end
 
+      it_behaves_like 'lfs object is created'
+
       it 'has the same oid' do
-        subject.execute(oid, download_link)
+        subject.execute
 
         expect(LfsObject.first.oid).to eq oid
       end
 
+      it 'has the same size' do
+        subject.execute
+
+        expect(LfsObject.first.size).to eq size
+      end
+
       it 'stores the content' do
-        subject.execute(oid, download_link)
+        subject.execute
 
         expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content
       end
     end
 
     context 'when file download fails' do
-      it 'no lfs object is created' do
-        expect { subject.execute(oid, download_link) }.to change { LfsObject.count }
+      before do
+        allow(Gitlab::HTTP).to receive(:get).and_return(code: 500, 'success?' => false)
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'raise StandardError exception' do
+        expect(subject).to receive(:download_and_save_file!).and_raise(StandardError)
+
+        subject.execute
+      end
+    end
+
+    context 'when downloaded lfs file has a different size' do
+      let(:size) { 1 }
+
+      before do
+        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'raise SizeError exception' do
+        expect(subject).to receive(:download_and_save_file!).and_raise(described_class::SizeError)
+
+        subject.execute
+      end
+    end
+
+    context 'when downloaded lfs file has a different oid' do
+      before do
+        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+        allow_any_instance_of(Digest::SHA256).to receive(:hexdigest).and_return('foobar')
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'raise OidError exception' do
+        expect(subject).to receive(:download_and_save_file!).and_raise(described_class::OidError)
+
+        subject.execute
       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) }
 
       before do
         WebMock.stub_request(:get, download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content)
       end
 
       it 'the request adds authorization headers' do
-        subject.execute(oid, download_link_with_credentials)
+        subject
       end
     end
 
     context 'when localhost requests are allowed' do
       let(:download_link) { 'http://192.168.2.120' }
+      let(:local_request_setting) { true }
 
       before do
-        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true)
+        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
       end
 
-      it 'downloads the file' do
-        expect(subject).to receive(:download_and_save_file).and_call_original
-
-        expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1)
-      end
+      it_behaves_like 'lfs object is created'
     end
 
     context 'when a bad URL is used' do
@@ -71,7 +159,9 @@ describe Projects::LfsPointers::LfsDownloadService do
 
       with_them do
         it 'does not download the file' do
-          expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
+          expect(subject).not_to receive(:download_lfs_file!)
+
+          expect { subject.execute }.not_to change { LfsObject.count }
         end
       end
     end
@@ -85,15 +175,11 @@ describe Projects::LfsPointers::LfsDownloadService do
             WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
           end
 
-          it 'does not follow the redirection' do
-            expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/)
-
-            expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
-          end
+          it_behaves_like 'no lfs object is created'
         end
       end
 
-      context 'that is valid' do
+      context 'that is not blocked' do
         let(:redirect_link) { "http://example.com/"}
 
         before do
@@ -101,21 +187,35 @@ describe Projects::LfsPointers::LfsDownloadService do
           WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content)
         end
 
-        it 'follows the redirection' do
-          expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1)
-        end
+        it_behaves_like 'lfs object is created'
+      end
+    end
+
+    context 'when the lfs object attributes are invalid' do
+      let(:oid) { 'foobar' }
+
+      before do
+        expect(lfs_object).to be_invalid
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'does not download the file' do
+        expect(subject).not_to receive(:download_lfs_file!)
+
+        subject.execute
       end
     end
 
     context 'when an lfs object with the same oid already exists'  do
       before do
-        create(:lfs_object, oid: 'oid')
+        create(:lfs_object, oid: oid)
       end
 
       it 'does not download the file' do
-        expect(subject).not_to receive(:download_and_save_file)
+        expect(subject).not_to receive(:download_lfs_file!)
 
-        subject.execute('oid', download_link)
+        subject.execute
       end
     end
   end