Added validations to prevent LFS object forgery
This commit is contained in:
parent
577812948d
commit
b3c13bbb3c
|
@ -0,0 +1,22 @@
|
||||||
|
# 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
|
|
@ -94,11 +94,11 @@ module Projects
|
||||||
|
|
||||||
return unless project.lfs_enabled?
|
return unless project.lfs_enabled?
|
||||||
|
|
||||||
oids_to_download = Projects::LfsPointers::LfsImportService.new(project).execute
|
lfs_objects_to_download = Projects::LfsPointers::LfsImportService.new(project).execute
|
||||||
download_service = Projects::LfsPointers::LfsDownloadService.new(project)
|
|
||||||
|
|
||||||
oids_to_download.each do |oid, link|
|
lfs_objects_to_download.each do |lfs_download_object|
|
||||||
download_service.execute(oid, link)
|
Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object)
|
||||||
|
.execute
|
||||||
end
|
end
|
||||||
rescue => e
|
rescue => e
|
||||||
# Right now, to avoid aborting the importing process, we silently fail
|
# Right now, to avoid aborting the importing process, we silently fail
|
||||||
|
|
|
@ -41,16 +41,17 @@ module Projects
|
||||||
end
|
end
|
||||||
|
|
||||||
def parse_response_links(objects_response)
|
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
|
begin
|
||||||
oid = entry['oid']
|
|
||||||
link = entry.dig('actions', DOWNLOAD_ACTION, 'href')
|
link = entry.dig('actions', DOWNLOAD_ACTION, 'href')
|
||||||
|
|
||||||
raise DownloadLinkNotFound unless link
|
raise DownloadLinkNotFound unless link
|
||||||
|
|
||||||
link_list[oid] = add_credentials(link)
|
link_list << LfsDownloadObject.new(oid: entry['oid'],
|
||||||
rescue DownloadLinkNotFound, URI::InvalidURIError
|
size: entry['size'],
|
||||||
Rails.logger.error("Link for Lfs Object with oid #{oid} not found or invalid.")
|
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
|
end
|
||||||
end
|
end
|
||||||
|
@ -70,7 +71,7 @@ module Projects
|
||||||
end
|
end
|
||||||
|
|
||||||
def add_credentials(link)
|
def add_credentials(link)
|
||||||
uri = URI.parse(link)
|
uri = Addressable::URI.parse(link)
|
||||||
|
|
||||||
if should_add_credentials?(uri)
|
if should_add_credentials?(uri)
|
||||||
uri.user = remote_uri.user
|
uri.user = remote_uri.user
|
||||||
|
|
|
@ -4,68 +4,93 @@
|
||||||
module Projects
|
module Projects
|
||||||
module LfsPointers
|
module LfsPointers
|
||||||
class LfsDownloadService < BaseService
|
class LfsDownloadService < BaseService
|
||||||
VALID_PROTOCOLS = %w[http https].freeze
|
SizeError = Class.new(StandardError)
|
||||||
|
OidError = Class.new(StandardError)
|
||||||
|
|
||||||
|
attr_reader :lfs_download_object
|
||||||
|
delegate :oid, :size, :credentials, :sanitized_url, to: :lfs_download_object, prefix: :lfs
|
||||||
|
|
||||||
|
def initialize(project, lfs_download_object)
|
||||||
|
super(project)
|
||||||
|
|
||||||
|
@lfs_download_object = lfs_download_object
|
||||||
|
end
|
||||||
|
|
||||||
# rubocop: disable CodeReuse/ActiveRecord
|
# rubocop: disable CodeReuse/ActiveRecord
|
||||||
def execute(oid, url)
|
def execute
|
||||||
return unless project&.lfs_enabled? && oid.present? && url.present?
|
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)
|
||||||
|
|
||||||
return if LfsObject.exists?(oid: oid)
|
wrap_download_errors do
|
||||||
|
download_lfs_file!
|
||||||
sanitized_uri = sanitize_url!(url)
|
|
||||||
|
|
||||||
with_tmp_file(oid) do |file|
|
|
||||||
download_and_save_file(file, sanitized_uri)
|
|
||||||
lfs_object = LfsObject.new(oid: oid, size: file.size, file: file)
|
|
||||||
|
|
||||||
project.all_lfs_objects << lfs_object
|
|
||||||
end
|
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
|
end
|
||||||
# rubocop: enable CodeReuse/ActiveRecord
|
# rubocop: enable CodeReuse/ActiveRecord
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def sanitize_url!(url)
|
def wrap_download_errors(&block)
|
||||||
Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri|
|
yield
|
||||||
# Just validate that HTTP/HTTPS protocols are used. The
|
rescue SizeError, OidError, StandardError => e
|
||||||
# subsequent Gitlab::HTTP.get call will do network checks
|
error("LFS file with oid #{lfs_oid} could't be downloaded from #{lfs_sanitized_url}: #{e.message}")
|
||||||
# based on the settings.
|
end
|
||||||
Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url,
|
|
||||||
protocols: VALID_PROTOCOLS)
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
def download_and_save_file(file, sanitized_uri)
|
def download_and_save_file!(file)
|
||||||
response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment|
|
digester = Digest::SHA256.new
|
||||||
|
response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment|
|
||||||
|
digester << fragment
|
||||||
file.write(fragment)
|
file.write(fragment)
|
||||||
|
|
||||||
|
raise_size_error! if file.size > lfs_size
|
||||||
end
|
end
|
||||||
|
|
||||||
raise StandardError, "Received error code #{response.code}" unless response.success?
|
raise StandardError, "Received error code #{response.code}" unless response.success?
|
||||||
|
|
||||||
|
raise_size_error! if file.size != lfs_size
|
||||||
|
raise_oid_error! if digester.hexdigest != lfs_oid
|
||||||
end
|
end
|
||||||
|
|
||||||
def headers(sanitized_uri)
|
def download_headers
|
||||||
query_options.tap do |headers|
|
{ stream_body: true }.tap do |headers|
|
||||||
credentials = sanitized_uri.credentials
|
if lfs_credentials[:user].present? || lfs_credentials[:password].present?
|
||||||
|
|
||||||
if credentials[:user].present? || credentials[:password].present?
|
|
||||||
# Using authentication headers in the request
|
# 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
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def query_options
|
def with_tmp_file
|
||||||
{ stream_body: true }
|
|
||||||
end
|
|
||||||
|
|
||||||
def with_tmp_file(oid)
|
|
||||||
create_tmp_storage_dir
|
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
|
end
|
||||||
|
|
||||||
def create_tmp_storage_dir
|
def create_tmp_storage_dir
|
||||||
|
@ -79,6 +104,20 @@ module Projects
|
||||||
def storage_dir
|
def storage_dir
|
||||||
@storage_dir ||= Gitlab.config.lfs.storage_path
|
@storage_dir ||= Gitlab.config.lfs.storage_path
|
||||||
end
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Add more LFS validations to prevent forgery
|
||||||
|
merge_request:
|
||||||
|
author:
|
||||||
|
type: security
|
|
@ -13,10 +13,12 @@ module Gitlab
|
||||||
@project = project
|
@project = project
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def lfs_download_object
|
||||||
|
LfsDownloadObject.new(oid: lfs_object.oid, size: lfs_object.size, link: lfs_object.link)
|
||||||
|
end
|
||||||
|
|
||||||
def execute
|
def execute
|
||||||
Projects::LfsPointers::LfsDownloadService
|
Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object).execute
|
||||||
.new(project)
|
|
||||||
.execute(lfs_object.oid, lfs_object.download_link)
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -9,11 +9,11 @@ module Gitlab
|
||||||
|
|
||||||
attr_reader :attributes
|
attr_reader :attributes
|
||||||
|
|
||||||
expose_attribute :oid, :download_link
|
expose_attribute :oid, :link, :size
|
||||||
|
|
||||||
# Builds a lfs_object
|
# Builds a lfs_object
|
||||||
def self.from_api_response(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
|
end
|
||||||
|
|
||||||
# Builds a new lfs_object using a Hash that was built from a JSON payload.
|
# Builds a new lfs_object using a Hash that was built from a JSON payload.
|
||||||
|
|
|
@ -2,20 +2,26 @@ require 'spec_helper'
|
||||||
|
|
||||||
describe Gitlab::GithubImport::Importer::LfsObjectImporter do
|
describe Gitlab::GithubImport::Importer::LfsObjectImporter do
|
||||||
let(:project) { create(:project) }
|
let(:project) { create(:project) }
|
||||||
let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" }
|
let(:lfs_attributes) do
|
||||||
|
{
|
||||||
let(:github_lfs_object) do
|
oid: 'oid',
|
||||||
Gitlab::GithubImport::Representation::LfsObject.new(
|
size: 1,
|
||||||
oid: 'oid', download_link: download_link
|
link: 'http://www.gitlab.com/lfs_objects/oid'
|
||||||
)
|
}
|
||||||
end
|
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) }
|
let(:importer) { described_class.new(github_lfs_object, project, nil) }
|
||||||
|
|
||||||
describe '#execute' do
|
describe '#execute' do
|
||||||
it 'calls the LfsDownloadService with the lfs object attributes' do
|
it 'calls the LfsDownloadService with the lfs object attributes' do
|
||||||
expect_any_instance_of(Projects::LfsPointers::LfsDownloadService)
|
allow(importer).to receive(:lfs_download_object).and_return(lfs_download_object)
|
||||||
.to receive(:execute).with('oid', download_link)
|
|
||||||
|
service = double
|
||||||
|
expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).with(project, lfs_download_object).and_return(service)
|
||||||
|
expect(service).to receive(:execute)
|
||||||
|
|
||||||
importer.execute
|
importer.execute
|
||||||
end
|
end
|
||||||
|
|
|
@ -5,7 +5,15 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do
|
||||||
let(:client) { double(:client) }
|
let(:client) { double(:client) }
|
||||||
let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" }
|
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
|
describe '#parallel?' do
|
||||||
it 'returns true when running in parallel mode' do
|
it 'returns true when running in parallel mode' do
|
||||||
|
@ -48,7 +56,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do
|
||||||
|
|
||||||
allow(importer)
|
allow(importer)
|
||||||
.to receive(:each_object_to_import)
|
.to receive(:each_object_to_import)
|
||||||
.and_yield(['oid', download_link])
|
.and_yield(lfs_download_object)
|
||||||
|
|
||||||
expect(Gitlab::GithubImport::Importer::LfsObjectImporter)
|
expect(Gitlab::GithubImport::Importer::LfsObjectImporter)
|
||||||
.to receive(:new)
|
.to receive(:new)
|
||||||
|
@ -71,7 +79,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do
|
||||||
|
|
||||||
allow(importer)
|
allow(importer)
|
||||||
.to receive(:each_object_to_import)
|
.to receive(:each_object_to_import)
|
||||||
.and_yield(github_lfs_object)
|
.and_yield(lfs_download_object)
|
||||||
|
|
||||||
expect(Gitlab::GithubImport::ImportLfsObjectWorker)
|
expect(Gitlab::GithubImport::ImportLfsObjectWorker)
|
||||||
.to receive(:perform_async)
|
.to receive(:perform_async)
|
||||||
|
|
|
@ -0,0 +1,68 @@
|
||||||
|
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
|
|
@ -152,8 +152,11 @@ describe Projects::ImportService do
|
||||||
|
|
||||||
it 'downloads lfs objects if lfs_enabled is enabled for project' do
|
it 'downloads lfs objects if lfs_enabled is enabled for project' do
|
||||||
allow(project).to receive(:lfs_enabled?).and_return(true)
|
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::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
|
subject.execute
|
||||||
end
|
end
|
||||||
|
@ -211,8 +214,10 @@ describe Projects::ImportService do
|
||||||
it 'does not have a custom repository importer downloads lfs objects' do
|
it 'does not have a custom repository importer downloads lfs objects' do
|
||||||
allow(Gitlab::GithubImport::ParallelImporter).to receive(:imports_repository?).and_return(false)
|
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::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
|
subject.execute
|
||||||
end
|
end
|
||||||
|
|
|
@ -37,8 +37,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
|
||||||
|
|
||||||
describe '#execute' do
|
describe '#execute' do
|
||||||
it 'retrieves each download link of every non existent lfs object' do
|
it 'retrieves each download link of every non existent lfs object' do
|
||||||
subject.execute(new_oids).each do |oid, link|
|
subject.execute(new_oids).each do |lfs_download_object|
|
||||||
expect(link).to eq "#{import_url}/gitlab-lfs/objects/#{oid}"
|
expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}"
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -50,8 +50,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
|
||||||
it 'adds credentials to the download_link' do
|
it 'adds credentials to the download_link' do
|
||||||
result = subject.execute(new_oids)
|
result = subject.execute(new_oids)
|
||||||
|
|
||||||
result.each do |oid, link|
|
result.each do |lfs_download_object|
|
||||||
expect(link.starts_with?('http://user:password@')).to be_truthy
|
expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_truthy
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -60,8 +60,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
|
||||||
it 'does not add any credentials' do
|
it 'does not add any credentials' do
|
||||||
result = subject.execute(new_oids)
|
result = subject.execute(new_oids)
|
||||||
|
|
||||||
result.each do |oid, link|
|
result.each do |lfs_download_object|
|
||||||
expect(link.starts_with?('http://user:password@')).to be_falsey
|
expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -74,8 +74,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
|
||||||
it 'downloads without any credentials' do
|
it 'downloads without any credentials' do
|
||||||
result = subject.execute(new_oids)
|
result = subject.execute(new_oids)
|
||||||
|
|
||||||
result.each do |oid, link|
|
result.each do |lfs_download_object|
|
||||||
expect(link.starts_with?('http://user:password@')).to be_falsey
|
expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -92,7 +92,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
|
||||||
|
|
||||||
describe '#parse_response_links' do
|
describe '#parse_response_links' do
|
||||||
it 'does not add oid entry if href not found' 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)
|
result = subject.send(:parse_response_links, invalid_object_response)
|
||||||
|
|
||||||
|
|
|
@ -2,68 +2,156 @@ require 'spec_helper'
|
||||||
|
|
||||||
describe Projects::LfsPointers::LfsDownloadService do
|
describe Projects::LfsPointers::LfsDownloadService do
|
||||||
let(:project) { create(:project) }
|
let(:project) { create(:project) }
|
||||||
let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' }
|
|
||||||
let(:download_link) { "http://gitlab.com/#{oid}" }
|
|
||||||
let(:lfs_content) { SecureRandom.random_bytes(10) }
|
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
|
before do
|
||||||
allow(project).to receive(:lfs_enabled?).and_return(true)
|
ApplicationSetting.create_from_defaults
|
||||||
WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
|
|
||||||
|
|
||||||
allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
|
stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting)
|
||||||
|
allow(project).to receive(:lfs_enabled?).and_return(true)
|
||||||
|
end
|
||||||
|
|
||||||
|
shared_examples 'lfs temporal file is removed' do
|
||||||
|
it do
|
||||||
|
subject.execute
|
||||||
|
|
||||||
|
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
|
end
|
||||||
|
|
||||||
describe '#execute' do
|
describe '#execute' do
|
||||||
context 'when file download succeeds' do
|
context 'when file download succeeds' do
|
||||||
it 'a new lfs object is created' do
|
before do
|
||||||
expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1)
|
WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it_behaves_like 'lfs object is created'
|
||||||
|
|
||||||
it 'has the same oid' do
|
it 'has the same oid' do
|
||||||
subject.execute(oid, download_link)
|
subject.execute
|
||||||
|
|
||||||
expect(LfsObject.first.oid).to eq oid
|
expect(LfsObject.first.oid).to eq oid
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'has the same size' do
|
||||||
|
subject.execute
|
||||||
|
|
||||||
|
expect(LfsObject.first.size).to eq size
|
||||||
|
end
|
||||||
|
|
||||||
it 'stores the content' do
|
it 'stores the content' do
|
||||||
subject.execute(oid, download_link)
|
subject.execute
|
||||||
|
|
||||||
expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content
|
expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when file download fails' do
|
context 'when file download fails' do
|
||||||
it 'no lfs object is created' do
|
before do
|
||||||
expect { subject.execute(oid, download_link) }.to change { LfsObject.count }
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when credentials present' do
|
context 'when credentials present' do
|
||||||
let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" }
|
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
|
before do
|
||||||
WebMock.stub_request(:get, download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content)
|
WebMock.stub_request(:get, download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'the request adds authorization headers' do
|
it 'the request adds authorization headers' do
|
||||||
subject.execute(oid, download_link_with_credentials)
|
subject
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when localhost requests are allowed' do
|
context 'when localhost requests are allowed' do
|
||||||
let(:download_link) { 'http://192.168.2.120' }
|
let(:download_link) { 'http://192.168.2.120' }
|
||||||
|
let(:local_request_setting) { true }
|
||||||
|
|
||||||
before do
|
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
|
end
|
||||||
|
|
||||||
it 'downloads the file' do
|
it_behaves_like 'lfs object is created'
|
||||||
expect(subject).to receive(:download_and_save_file).and_call_original
|
|
||||||
|
|
||||||
expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1)
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when a bad URL is used' do
|
context 'when a bad URL is used' do
|
||||||
|
@ -71,7 +159,9 @@ describe Projects::LfsPointers::LfsDownloadService do
|
||||||
|
|
||||||
with_them do
|
with_them do
|
||||||
it 'does not download the file' 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
|
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 })
|
WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'does not follow the redirection' do
|
it_behaves_like 'no lfs object is created'
|
||||||
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
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'that is valid' do
|
context 'that is not blocked' do
|
||||||
let(:redirect_link) { "http://example.com/"}
|
let(:redirect_link) { "http://example.com/"}
|
||||||
|
|
||||||
before do
|
before do
|
||||||
|
@ -101,21 +187,35 @@ describe Projects::LfsPointers::LfsDownloadService do
|
||||||
WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content)
|
WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'follows the redirection' do
|
it_behaves_like 'lfs object is created'
|
||||||
expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1)
|
end
|
||||||
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
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when an lfs object with the same oid already exists' do
|
context 'when an lfs object with the same oid already exists' do
|
||||||
before do
|
before do
|
||||||
create(:lfs_object, oid: 'oid')
|
create(:lfs_object, oid: oid)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'does not download the file' do
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue