diff options
author | Francisco Javier López <fjlopez@gitlab.com> | 2018-12-14 18:16:03 +0100 |
---|---|---|
committer | Francisco Javier López <fjlopez@gitlab.com> | 2018-12-18 09:01:17 +0100 |
commit | b7a9c26a5bd9b33de9193ba3fd88e3c5d4a2d996 (patch) | |
tree | 33ab867a60aa4efc3668f3dd89bafd738f48b59b /spec/services/projects/lfs_pointers | |
parent | 7304d66b8728b16916d90f80abe4b25f03446f29 (diff) | |
download | gitlab-ce-b7a9c26a5bd9b33de9193ba3fd88e3c5d4a2d996.tar.gz |
Fixed SSRF in project imports with LFS
Diffstat (limited to 'spec/services/projects/lfs_pointers')
-rw-r--r-- | spec/services/projects/lfs_pointers/lfs_download_service_spec.rb | 59 |
1 files changed, 50 insertions, 9 deletions
diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index d7d7f1874eb..95c9b6e63b8 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do let(:project) { create(:project) } let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' } let(:download_link) { "http://gitlab.com/#{oid}" } - let(:lfs_content) do - <<~HEREDOC - whatever - HEREDOC - end + let(:lfs_content) { SecureRandom.random_bytes(10) } subject { described_class.new(project) } before do allow(project).to receive(:lfs_enabled?).and_return(true) 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) end describe '#execute' do @@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do it 'stores the content' do subject.execute(oid, download_link) - expect(File.read(LfsObject.first.file.file.file)).to eq lfs_content + expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content end end @@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when localhost requests are allowed' do + let(:download_link) { 'http://192.168.2.120' } + + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + 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 + end + context 'when a bad URL is used' do - where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2']) + where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) with_them do it 'does not download the file' do - expect(subject).not_to receive(:download_and_save_file) - expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } end end end + context 'when the URL points to a redirected URL' do + context 'that is blocked' do + where(redirect_link: ['ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) + + with_them do + before 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 + end + end + + context 'that is valid' do + let(:redirect_link) { "http://example.com/"} + + before do + WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + 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 + end + end + context 'when an lfs object with the same oid already exists' do before do create(:lfs_object, oid: 'oid') |