diff options
Diffstat (limited to 'spec/lib')
-rw-r--r-- | spec/lib/gitlab/ci/artifact_file_reader_spec.rb | 124 | ||||
-rw-r--r-- | spec/lib/safe_zip/entry_spec.rb | 35 | ||||
-rw-r--r-- | spec/lib/safe_zip/extract_params_spec.rb | 35 | ||||
-rw-r--r-- | spec/lib/safe_zip/extract_spec.rb | 55 |
4 files changed, 193 insertions, 56 deletions
diff --git a/spec/lib/gitlab/ci/artifact_file_reader_spec.rb b/spec/lib/gitlab/ci/artifact_file_reader_spec.rb index e982f0eb015..813dc15e79f 100644 --- a/spec/lib/gitlab/ci/artifact_file_reader_spec.rb +++ b/spec/lib/gitlab/ci/artifact_file_reader_spec.rb @@ -10,71 +10,117 @@ RSpec.describe Gitlab::Ci::ArtifactFileReader do subject { described_class.new(job).read(path) } context 'when job has artifacts and metadata' do - let!(:artifacts) { create(:ci_job_artifact, :archive, job: job) } - let!(:metadata) { create(:ci_job_artifact, :metadata, job: job) } + shared_examples 'extracting job artifact archive' do + it 'returns the content at the path' do + is_expected.to be_present + expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom') + end - it 'returns the content at the path' do - is_expected.to be_present - expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom') - end + context 'when path does not exist' do + let(:path) { 'file/does/not/exist.txt' } + let(:expected_error) do + "Path `#{path}` does not exist inside the `#{job.name}` artifacts archive!" + end - context 'when path does not exist' do - let(:path) { 'file/does/not/exist.txt' } - let(:expected_error) do - "Path `#{path}` does not exist inside the `#{job.name}` artifacts archive!" + it 'raises an error' do + expect { subject }.to raise_error(described_class::Error, expected_error) + end end - it 'raises an error' do - expect { subject }.to raise_error(described_class::Error, expected_error) + context 'when path points to a directory' do + let(:path) { 'other_artifacts_0.1.2' } + let(:expected_error) do + "Path `#{path}` was expected to be a file but it was a directory!" + end + + it 'raises an error' do + expect { subject }.to raise_error(described_class::Error, expected_error) + end end - end - context 'when path points to a directory' do - let(:path) { 'other_artifacts_0.1.2' } - let(:expected_error) do - "Path `#{path}` was expected to be a file but it was a directory!" + context 'when path is nested' do + # path exists in ci_build_artifacts.zip + let(:path) { 'other_artifacts_0.1.2/doc_sample.txt' } + + it 'returns the content at the nested path' do + is_expected.to be_present + end end - it 'raises an error' do - expect { subject }.to raise_error(described_class::Error, expected_error) + context 'when artifact archive size is greater than the limit' do + let(:expected_error) do + "Artifacts archive for job `#{job.name}` is too large: max 1 KB" + end + + before do + stub_const("#{described_class}::MAX_ARCHIVE_SIZE", 1.kilobyte) + end + + it 'raises an error' do + expect { subject }.to raise_error(described_class::Error, expected_error) + end end - end - context 'when path is nested' do - # path exists in ci_build_artifacts.zip - let(:path) { 'other_artifacts_0.1.2/doc_sample.txt' } + context 'when metadata entry shows size greater than the limit' do + let(:expected_error) do + "Artifacts archive for job `#{job.name}` is too large: max 5 MB" + end - it 'returns the content at the nested path' do - is_expected.to be_present + before do + expect_next_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Entry) do |entry| + expect(entry).to receive(:total_size).and_return(10.megabytes) + end + end + + it 'raises an error' do + expect { subject }.to raise_error(described_class::Error, expected_error) + end end end - context 'when artifact archive size is greater than the limit' do - let(:expected_error) do - "Artifacts archive for job `#{job.name}` is too large: max 1 KB" - end + context 'when job artifact is on local storage' do + let!(:artifacts) { create(:ci_job_artifact, :archive, job: job) } + let!(:metadata) { create(:ci_job_artifact, :metadata, job: job) } + + it_behaves_like 'extracting job artifact archive' + end + context 'when job artifact is on remote storage' do before do - stub_const("#{described_class}::MAX_ARCHIVE_SIZE", 1.kilobyte) + stub_artifacts_object_storage + stub_request(:get, %r{https://artifacts.+ci_build_artifacts\.zip}) + .to_return( + status: 200, + body: File.open(Rails.root.join('spec/fixtures/ci_build_artifacts.zip')), + headers: {} + ) + stub_request(:get, %r{https://artifacts.+ci_build_artifacts_metadata}) + .to_return( + status: 200, + body: File.open(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz')), + headers: {} + ) end - it 'raises an error' do - expect { subject }.to raise_error(described_class::Error, expected_error) - end + let!(:artifacts) { create(:ci_job_artifact, :archive, :remote_store, job: job) } + let!(:metadata) { create(:ci_job_artifact, :metadata, :remote_store, job: job) } + + it_behaves_like 'extracting job artifact archive' end - context 'when metadata entry shows size greater than the limit' do - let(:expected_error) do - "Artifacts archive for job `#{job.name}` is too large: max 5 MB" - end + context 'when extracting job artifact raises entry size error' do + let!(:artifacts) { create(:ci_job_artifact, :archive, job: job) } + let!(:metadata) { create(:ci_job_artifact, :metadata, job: job) } before do - expect_next_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Entry) do |entry| - expect(entry).to receive(:total_size).and_return(10.megabytes) + allow_next_instance_of(SafeZip::Extract, anything) do |extractor| + allow(extractor).to receive(:extract).and_raise(SafeZip::Extract::EntrySizeError) end end it 'raises an error' do + expected_error = "Path `#{path}` has invalid size in the zip!" + expect { subject }.to raise_error(described_class::Error, expected_error) end end diff --git a/spec/lib/safe_zip/entry_spec.rb b/spec/lib/safe_zip/entry_spec.rb index 9929b8073a0..8d49e2bcece 100644 --- a/spec/lib/safe_zip/entry_spec.rb +++ b/spec/lib/safe_zip/entry_spec.rb @@ -5,12 +5,13 @@ require 'spec_helper' RSpec.describe SafeZip::Entry do let(:target_path) { Dir.mktmpdir('safe-zip') } let(:directories) { %w(public folder/with/subfolder) } - let(:params) { SafeZip::ExtractParams.new(directories: directories, to: target_path) } + let(:files) { %w(public/index.html public/assets/image.png) } + let(:params) { SafeZip::ExtractParams.new(directories: directories, files: files, to: target_path) } let(:entry) { described_class.new(zip_archive, zip_entry, params) } let(:entry_name) { 'public/folder/index.html' } let(:entry_path_dir) { File.join(target_path, File.dirname(entry_name)) } - let(:entry_path) { File.join(target_path, entry_name) } + let(:entry_path) { File.join(File.realpath(target_path), entry_name) } let(:zip_archive) { double } let(:zip_entry) do @@ -28,7 +29,7 @@ RSpec.describe SafeZip::Entry do describe '#path_dir' do subject { entry.path_dir } - it { is_expected.to eq(target_path + '/public/folder') } + it { is_expected.to eq(File.realpath(target_path) + '/public/folder') } end describe '#exist?' do @@ -51,6 +52,9 @@ RSpec.describe SafeZip::Entry do subject { entry.extract } context 'when entry does not match the filtered directories' do + let(:directories) { %w(public folder/with/subfolder) } + let(:files) { [] } + using RSpec::Parameterized::TableSyntax where(:entry_name) do @@ -70,7 +74,30 @@ RSpec.describe SafeZip::Entry do end end - context 'when entry does exist' do + context 'when entry does not match the filtered files' do + let(:directories) { [] } + let(:files) { %w(public/index.html public/assets/image.png) } + + using RSpec::Parameterized::TableSyntax + + where(:entry_name) do + [ + 'assets/folder/index.html', + 'public/../folder/index.html', + 'public/../../../../../index.html', + '../../../../../public/index.html', + '/etc/passwd' + ] + end + + with_them do + it 'does not extract file' do + is_expected.to be_falsey + end + end + end + + context 'when there is an existing extracted entry' do before do create_entry end diff --git a/spec/lib/safe_zip/extract_params_spec.rb b/spec/lib/safe_zip/extract_params_spec.rb index 880c4358663..0ebfb7430c5 100644 --- a/spec/lib/safe_zip/extract_params_spec.rb +++ b/spec/lib/safe_zip/extract_params_spec.rb @@ -4,8 +4,10 @@ require 'spec_helper' RSpec.describe SafeZip::ExtractParams do let(:target_path) { Dir.mktmpdir("safe-zip") } - let(:params) { described_class.new(directories: directories, to: target_path) } + let(:real_target_path) { File.realpath(target_path) } + let(:params) { described_class.new(directories: directories, files: files, to: target_path) } let(:directories) { %w(public folder/with/subfolder) } + let(:files) { %w(public/index.html public/assets/image.png) } after do FileUtils.remove_entry_secure(target_path) @@ -14,13 +16,13 @@ RSpec.describe SafeZip::ExtractParams do describe '#extract_path' do subject { params.extract_path } - it { is_expected.to eq(target_path) } + it { is_expected.to eq(real_target_path) } end describe '#matching_target_directory' do using RSpec::Parameterized::TableSyntax - subject { params.matching_target_directory(target_path + path) } + subject { params.matching_target_directory(real_target_path + path) } where(:path, :result) do '/public/index.html' | '/public/' @@ -30,7 +32,7 @@ RSpec.describe SafeZip::ExtractParams do end with_them do - it { is_expected.to eq(result ? target_path + result : nil) } + it { is_expected.to eq(result ? real_target_path + result : nil) } end end @@ -38,7 +40,7 @@ RSpec.describe SafeZip::ExtractParams do subject { params.target_directories } it 'starts with target_path' do - is_expected.to all(start_with(target_path + '/')) + is_expected.to all(start_with(real_target_path + '/')) end it 'ends with / for all paths' do @@ -53,4 +55,27 @@ RSpec.describe SafeZip::ExtractParams do is_expected.to all(end_with('/*')) end end + + describe '#matching_target_file' do + using RSpec::Parameterized::TableSyntax + + subject { params.matching_target_file(real_target_path + path) } + + where(:path, :result) do + '/public/index.html' | true + '/non/existing/path' | false + '/public/' | false + '/folder/with/index.html' | false + end + + with_them do + it { is_expected.to eq(result) } + end + end + + context 'when directories and files are empty' do + it 'is invalid' do + expect { described_class.new(to: target_path) }.to raise_error(ArgumentError, /directories or files are required/) + end + end end diff --git a/spec/lib/safe_zip/extract_spec.rb b/spec/lib/safe_zip/extract_spec.rb index 443430b267d..c727475e271 100644 --- a/spec/lib/safe_zip/extract_spec.rb +++ b/spec/lib/safe_zip/extract_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe SafeZip::Extract do let(:target_path) { Dir.mktmpdir('safe-zip') } let(:directories) { %w(public) } + let(:files) { %w(public/index.html) } let(:object) { described_class.new(archive) } let(:archive) { Rails.root.join('spec', 'fixtures', 'safe_zip', archive_name) } @@ -13,20 +14,36 @@ RSpec.describe SafeZip::Extract do end describe '#extract' do - subject { object.extract(directories: directories, to: target_path) } + subject { object.extract(directories: directories, files: files, to: target_path) } shared_examples 'extracts archive' do - it 'does extract archive' do - subject + context 'when specifying directories' do + subject { object.extract(directories: directories, to: target_path) } - expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true) - expect(File.exist?(File.join(target_path, 'source'))).to eq(false) + it 'does extract archive' do + subject + + expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true) + expect(File.exist?(File.join(target_path, 'public', 'assets', 'image.png'))).to eq(true) + expect(File.exist?(File.join(target_path, 'source'))).to eq(false) + end + end + + context 'when specifying files' do + subject { object.extract(files: files, to: target_path) } + + it 'does extract archive' do + subject + + expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true) + expect(File.exist?(File.join(target_path, 'public', 'assets', 'image.png'))).to eq(false) + end end end shared_examples 'fails to extract archive' do it 'does not extract archive' do - expect { subject }.to raise_error(SafeZip::Extract::Error) + expect { subject }.to raise_error(SafeZip::Extract::Error, including(error_message)) end end @@ -38,9 +55,18 @@ RSpec.describe SafeZip::Extract do end end - %w(invalid-symlink-does-not-exist.zip invalid-symlinks-outside.zip).each do |name| - context "when using #{name} archive" do + context 'when zip files are invalid' do + using RSpec::Parameterized::TableSyntax + + where(:name, :message) do + 'invalid-symlink-does-not-exist.zip' | 'does not exist' + 'invalid-symlinks-outside.zip' | 'Symlink cannot be created' + 'invalid-unexpected-large.zip' | 'larger when inflated' + end + + with_them do let(:archive_name) { name } + let(:error_message) { message } it_behaves_like 'fails to extract archive' end @@ -49,6 +75,19 @@ RSpec.describe SafeZip::Extract do context 'when no matching directories are found' do let(:archive_name) { 'valid-simple.zip' } let(:directories) { %w(non/existing) } + let(:error_message) { 'No entries extracted' } + + subject { object.extract(directories: directories, to: target_path) } + + it_behaves_like 'fails to extract archive' + end + + context 'when no matching files are found' do + let(:archive_name) { 'valid-simple.zip' } + let(:files) { %w(non/existing) } + let(:error_message) { 'No entries extracted' } + + subject { object.extract(files: files, to: target_path) } it_behaves_like 'fails to extract archive' end |