From e92925533667e147ff34cf1e9b8af21680c8c7d4 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 30 Jan 2023 09:13:00 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-8-stable-ee --- app/models/concerns/issuable.rb | 19 ++-- app/models/concerns/sanitizable.rb | 9 ++ app/models/namespace_setting.rb | 13 +-- .../packages/helm/extract_file_metadata_service.rb | 5 + lib/gitlab/ci/artifact_file_reader.rb | 23 ++-- lib/gitlab/utils.rb | 3 +- lib/safe_zip/entry.rb | 10 +- lib/safe_zip/extract.rb | 1 + lib/safe_zip/extract_params.rb | 24 +++- spec/fixtures/packages/helm/corrupted_chart.tgz | Bin 0 -> 2084191 bytes .../fixtures/safe_zip/invalid-unexpected-large.zip | Bin 0 -> 376 bytes spec/fixtures/safe_zip/valid-symlinks-first.zip | Bin 528 -> 1297 bytes spec/lib/gitlab/ci/artifact_file_reader_spec.rb | 124 ++++++++++++++------- spec/lib/safe_zip/entry_spec.rb | 35 +++++- spec/lib/safe_zip/extract_params_spec.rb | 35 +++++- spec/lib/safe_zip/extract_spec.rb | 55 +++++++-- spec/models/concerns/issuable_spec.rb | 1 - spec/models/concerns/sanitizable_spec.rb | 53 ++++++++- spec/models/namespace_setting_spec.rb | 6 +- .../helm/extract_file_metadata_service_spec.rb | 13 +++ .../models/concerns/issuable_shared_examples.rb | 103 ++++++++++++++--- .../models/concerns/sanitizable_shared_examples.rb | 19 +++- 22 files changed, 440 insertions(+), 111 deletions(-) create mode 100644 spec/fixtures/packages/helm/corrupted_chart.tgz create mode 100644 spec/fixtures/safe_zip/invalid-unexpected-large.zip diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 9f0cd96a8f8..50696c7b5e1 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -92,10 +92,9 @@ module Issuable validates :author, presence: true validates :title, presence: true, length: { maximum: TITLE_LENGTH_MAX } - # we validate the description against DESCRIPTION_LENGTH_MAX only for Issuables being created - # to avoid breaking the existing Issuables which may have their descriptions longer - validates :description, length: { maximum: DESCRIPTION_LENGTH_MAX }, allow_blank: true, on: :create - validate :description_max_length_for_new_records_is_valid, on: :update + # we validate the description against DESCRIPTION_LENGTH_MAX only for Issuables being created and on updates if + # the description changes to avoid breaking the existing Issuables which may have their descriptions longer + validates :description, bytesize: { maximum: -> { DESCRIPTION_LENGTH_MAX } }, if: :validate_description_length? validate :validate_assignee_size_length, unless: :importing? before_validation :truncate_description_on_import! @@ -229,10 +228,14 @@ module Issuable private - def description_max_length_for_new_records_is_valid - if new_record? && description.length > Issuable::DESCRIPTION_LENGTH_MAX - errors.add(:description, :too_long, count: Issuable::DESCRIPTION_LENGTH_MAX) - end + def validate_description_length? + return false unless description_changed? + + previous_description = changes['description'].first + # previous_description will be nil for new records + return true if previous_description.blank? + + previous_description.bytesize <= DESCRIPTION_LENGTH_MAX end def truncate_description_on_import! diff --git a/app/models/concerns/sanitizable.rb b/app/models/concerns/sanitizable.rb index 05756beb404..653d7a4875d 100644 --- a/app/models/concerns/sanitizable.rb +++ b/app/models/concerns/sanitizable.rb @@ -45,6 +45,15 @@ module Sanitizable unless input.to_s == CGI.unescapeHTML(input.to_s) record.errors.add(attr, 'cannot contain escaped HTML entities') end + + # This method raises an exception on failure so perform this + # last if multiple errors should be returned. + Gitlab::Utils.check_path_traversal!(input.to_s) + + rescue Gitlab::Utils::DoubleEncodingError + record.errors.add(attr, 'cannot contain escaped components') + rescue Gitlab::Utils::PathTraversalAttackError + record.errors.add(attr, "cannot contain a path traversal component") end end end diff --git a/app/models/namespace_setting.rb b/app/models/namespace_setting.rb index 7f65fb3a378..aeb4d7a5694 100644 --- a/app/models/namespace_setting.rb +++ b/app/models/namespace_setting.rb @@ -14,10 +14,11 @@ class NamespaceSetting < ApplicationRecord validates :enabled_git_access_protocol, inclusion: { in: enabled_git_access_protocols.keys } - validate :default_branch_name_content validate :allow_mfa_for_group validate :allow_resource_access_token_creation_for_group + sanitizes! :default_branch_name + before_validation :normalize_default_branch_name chronic_duration_attr :runner_token_expiration_interval_human_readable, :runner_token_expiration_interval @@ -45,8 +46,6 @@ class NamespaceSetting < ApplicationRecord NAMESPACE_SETTINGS_PARAMS end - sanitizes! :default_branch_name - def prevent_sharing_groups_outside_hierarchy return super if namespace.root? @@ -85,14 +84,6 @@ class NamespaceSetting < ApplicationRecord self.default_branch_name = default_branch_name.presence end - def default_branch_name_content - return if default_branch_name.nil? - - if default_branch_name.blank? - errors.add(:default_branch_name, "can not be an empty string") - end - end - def allow_mfa_for_group if namespace&.subgroup? && allow_mfa_for_subgroups == false errors.add(:allow_mfa_for_subgroups, _('is not allowed since the group is not top-level group.')) diff --git a/app/services/packages/helm/extract_file_metadata_service.rb b/app/services/packages/helm/extract_file_metadata_service.rb index e7373d8ea8f..77efa65f1d1 100644 --- a/app/services/packages/helm/extract_file_metadata_service.rb +++ b/app/services/packages/helm/extract_file_metadata_service.rb @@ -7,6 +7,10 @@ module Packages class ExtractFileMetadataService ExtractionError = Class.new(StandardError) + # Charts must be smaller than 1M because of the storage limitations of Kubernetes objects. + # based on https://helm.sh/docs/chart_template_guide/accessing_files/ + MAX_FILE_SIZE = 1.megabytes.freeze + def initialize(package_file) @package_file = package_file end @@ -42,6 +46,7 @@ module Packages end raise ExtractionError, 'Chart.yaml not found within a directory' unless chart_yaml + raise ExtractionError, 'Chart.yaml too big' if chart_yaml.size > MAX_FILE_SIZE chart_yaml.read ensure diff --git a/lib/gitlab/ci/artifact_file_reader.rb b/lib/gitlab/ci/artifact_file_reader.rb index b0fad026ec5..2eb8df01d58 100644 --- a/lib/gitlab/ci/artifact_file_reader.rb +++ b/lib/gitlab/ci/artifact_file_reader.rb @@ -9,6 +9,7 @@ module Gitlab Error = Class.new(StandardError) MAX_ARCHIVE_SIZE = 5.megabytes + TMP_ARTIFACT_EXTRACTION_DIR = "extracted_artifacts_job_%{id}" def initialize(job) @job = job @@ -45,20 +46,20 @@ module Gitlab end def read_zip_file!(file_path) - job.artifacts_file.use_open_file do |file| - zip_file = Zip::File.new(file, false, true) - entry = zip_file.find_entry(file_path) + dir_name = format(TMP_ARTIFACT_EXTRACTION_DIR, id: job.id.to_i) - unless entry - raise Error, "Path `#{file_path}` does not exist inside the `#{job.name}` artifacts archive!" + job.artifacts_file.use_open_file(unlink_early: false) do |tmp_open_file| + Dir.mktmpdir(dir_name) do |tmp_dir| + SafeZip::Extract.new(tmp_open_file.file_path).extract(files: [file_path], to: tmp_dir) + File.read(File.join(tmp_dir, file_path)) end - - if entry.name_is_directory? - raise Error, "Path `#{file_path}` was expected to be a file but it was a directory!" - end - - zip_file.read(entry) end + rescue SafeZip::Extract::NoMatchingError + raise Error, "Path `#{file_path}` does not exist inside the `#{job.name}` artifacts archive!" + rescue SafeZip::Extract::EntrySizeError + raise Error, "Path `#{file_path}` has invalid size in the zip!" + rescue Errno::EISDIR + raise Error, "Path `#{file_path}` was expected to be a file but it was a directory!" end def max_archive_size_in_mb diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index d3055569ece..8bd4cd2401d 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -4,6 +4,7 @@ module Gitlab module Utils extend self PathTraversalAttackError ||= Class.new(StandardError) + DoubleEncodingError ||= Class.new(StandardError) private_class_method def logger @logger ||= Gitlab::AppLogger @@ -55,7 +56,7 @@ module Gitlab def decode_path(encoded_path) decoded = CGI.unescape(encoded_path) if decoded != CGI.unescape(decoded) - raise StandardError, "path #{encoded_path} is not allowed" + raise DoubleEncodingError, "path #{encoded_path} is not allowed" end decoded diff --git a/lib/safe_zip/entry.rb b/lib/safe_zip/entry.rb index 52d70e83154..88647b9b1eb 100644 --- a/lib/safe_zip/entry.rb +++ b/lib/safe_zip/entry.rb @@ -25,8 +25,8 @@ module SafeZip end def extract - # do not extract if file is not part of target directory - return false unless matching_target_directory + # do not extract if file is not part of target directory or target file + return false unless matching_target_directory || matching_target_file # do not overwrite existing file raise SafeZip::Extract::AlreadyExistsError, "File already exists #{zip_entry.name}" if exist? @@ -44,6 +44,8 @@ module SafeZip end rescue SafeZip::Extract::Error raise + rescue Zip::EntrySizeError => e + raise SafeZip::Extract::EntrySizeError, e.message rescue StandardError => e raise SafeZip::Extract::ExtractError, e.message end @@ -84,6 +86,10 @@ module SafeZip params.matching_target_directory(path) end + def matching_target_file + params.matching_target_file(path) + end + def read_symlink zip_archive.read(zip_entry) end diff --git a/lib/safe_zip/extract.rb b/lib/safe_zip/extract.rb index 74df7895afe..b86941e6bea 100644 --- a/lib/safe_zip/extract.rb +++ b/lib/safe_zip/extract.rb @@ -6,6 +6,7 @@ module SafeZip PermissionDeniedError = Class.new(Error) SymlinkSourceDoesNotExistError = Class.new(Error) UnsupportedEntryError = Class.new(Error) + EntrySizeError = Class.new(Error) AlreadyExistsError = Class.new(Error) NoMatchingError = Class.new(Error) ExtractError = Class.new(Error) diff --git a/lib/safe_zip/extract_params.rb b/lib/safe_zip/extract_params.rb index bd3b788bac9..96881ad1abc 100644 --- a/lib/safe_zip/extract_params.rb +++ b/lib/safe_zip/extract_params.rb @@ -4,11 +4,13 @@ module SafeZip class ExtractParams include Gitlab::Utils::StrongMemoize - attr_reader :directories, :extract_path + attr_reader :directories, :files, :extract_path - def initialize(directories:, to:) + def initialize(to:, directories: [], files: []) @directories = directories + @files = files @extract_path = ::File.realpath(to) + validate! end def matching_target_directory(path) @@ -32,5 +34,23 @@ module SafeZip end end end + + def matching_target_file(path) + target_files.include?(path) + end + + private + + def target_files + strong_memoize(:target_files) do + files.map do |file| + ::File.join(extract_path, file) + end + end + end + + def validate! + raise ArgumentError, 'Either directories or files are required' if directories.empty? && files.empty? + end end end diff --git a/spec/fixtures/packages/helm/corrupted_chart.tgz b/spec/fixtures/packages/helm/corrupted_chart.tgz new file mode 100644 index 00000000000..b2ac93b271e Binary files /dev/null and b/spec/fixtures/packages/helm/corrupted_chart.tgz differ diff --git a/spec/fixtures/safe_zip/invalid-unexpected-large.zip b/spec/fixtures/safe_zip/invalid-unexpected-large.zip new file mode 100644 index 00000000000..3005da8c779 Binary files /dev/null and b/spec/fixtures/safe_zip/invalid-unexpected-large.zip differ diff --git a/spec/fixtures/safe_zip/valid-symlinks-first.zip b/spec/fixtures/safe_zip/valid-symlinks-first.zip index f5952ef71c9..1d7ecfd7bed 100644 Binary files a/spec/fixtures/safe_zip/valid-symlinks-first.zip and b/spec/fixtures/safe_zip/valid-symlinks-first.zip differ 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 diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index e553e34ab51..206b3ae61cf 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -65,7 +65,6 @@ RSpec.describe Issuable do it { is_expected.to validate_presence_of(:author) } it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_length_of(:title).is_at_most(described_class::TITLE_LENGTH_MAX) } - it { is_expected.to validate_length_of(:description).is_at_most(described_class::DESCRIPTION_LENGTH_MAX).on(:create) } it_behaves_like 'validates description length with custom validation' do before do diff --git a/spec/models/concerns/sanitizable_spec.rb b/spec/models/concerns/sanitizable_spec.rb index 4a1d463d666..be7169f8dca 100644 --- a/spec/models/concerns/sanitizable_spec.rb +++ b/spec/models/concerns/sanitizable_spec.rb @@ -75,7 +75,58 @@ RSpec.describe Sanitizable do it 'is not valid', :aggregate_failures do expect(record).not_to be_valid - expect(record.errors.full_messages).to include('Name cannot contain escaped HTML entities') + expect(record.errors.full_messages).to contain_exactly( + 'Name cannot contain escaped HTML entities', + 'Description cannot contain escaped HTML entities' + ) + end + end + + context 'when input contains double-escaped data' do + let_it_be(:input) do + '%2526lt%253Bscript%2526gt%253Balert%25281%2529%2526lt%253B%252Fscript%2526gt%253B' + end + + it_behaves_like 'noop' + + it 'is not valid', :aggregate_failures do + expect(record).not_to be_valid + expect(record.errors.full_messages).to contain_exactly( + 'Name cannot contain escaped components', + 'Description cannot contain escaped components' + ) + end + end + + context 'when input contains a path traversal attempt' do + let_it_be(:input) { 'main../../../../../../api/v4/projects/1/import_project_members/2' } + + it_behaves_like 'noop' + + it 'is not valid', :aggregate_failures do + expect(record).not_to be_valid + expect(record.errors.full_messages).to contain_exactly( + 'Name cannot contain a path traversal component', + 'Description cannot contain a path traversal component' + ) + end + end + + context 'when input contains both path traversal attempt and pre-escaped entities' do + let_it_be(:input) do + 'main../../../../../../api/v4/projects/1/import_project_members/2<script>alert(1)</script>' + end + + it_behaves_like 'noop' + + it 'is not valid', :aggregate_failures do + expect(record).not_to be_valid + expect(record.errors.full_messages).to contain_exactly( + 'Name cannot contain a path traversal component', + 'Name cannot contain escaped HTML entities', + 'Description cannot contain a path traversal component', + 'Description cannot contain escaped HTML entities' + ) end end end diff --git a/spec/models/namespace_setting_spec.rb b/spec/models/namespace_setting_spec.rb index 0bf6fdf4fa0..15b80749aa2 100644 --- a/spec/models/namespace_setting_spec.rb +++ b/spec/models/namespace_setting_spec.rb @@ -18,7 +18,7 @@ RSpec.describe NamespaceSetting, feature_category: :subgroups, type: :model do describe "#default_branch_name_content" do let_it_be(:group) { create(:group) } - let(:namespace_settings) { group.namespace_settings } + subject(:namespace_settings) { group.namespace_settings } shared_examples "doesn't return an error" do it "doesn't return an error" do @@ -28,6 +28,10 @@ RSpec.describe NamespaceSetting, feature_category: :subgroups, type: :model do end context "when not set" do + before do + namespace_settings.default_branch_name = nil + end + it_behaves_like "doesn't return an error" end diff --git a/spec/services/packages/helm/extract_file_metadata_service_spec.rb b/spec/services/packages/helm/extract_file_metadata_service_spec.rb index 273f679b736..f4c61c12344 100644 --- a/spec/services/packages/helm/extract_file_metadata_service_spec.rb +++ b/spec/services/packages/helm/extract_file_metadata_service_spec.rb @@ -54,4 +54,17 @@ RSpec.describe Packages::Helm::ExtractFileMetadataService do it { expect { subject }.to raise_error(described_class::ExtractionError, 'Error while parsing Chart.yaml: (): did not find expected node content while parsing a flow node at line 2 column 1') } end + + context 'with a corrupted Chart.yaml of incorrect size' do + let(:helm_fixture_path) { expand_fixture_path('packages/helm/corrupted_chart.tgz') } + let(:expected_error_message) { 'Chart.yaml too big' } + + before do + allow(Zlib::GzipReader).to receive(:new).and_return(Zlib::GzipReader.new(File.open(helm_fixture_path))) + end + + it 'raises an error with the expected message' do + expect { subject }.to raise_error(::Packages::Helm::ExtractFileMetadataService::ExtractionError, expected_error_message) + end + end end diff --git a/spec/support/shared_examples/models/concerns/issuable_shared_examples.rb b/spec/support/shared_examples/models/concerns/issuable_shared_examples.rb index 3a407088997..f49ec906382 100644 --- a/spec/support/shared_examples/models/concerns/issuable_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/issuable_shared_examples.rb @@ -10,40 +10,111 @@ RSpec.shared_examples 'matches_cross_reference_regex? fails fast' do end RSpec.shared_examples 'validates description length with custom validation' do - let(:issuable) { build(:issue, description: 'x' * (::Issuable::DESCRIPTION_LENGTH_MAX + 1)) } - let(:context) { :update } + let(:invalid_description) { 'x' * (::Issuable::DESCRIPTION_LENGTH_MAX + 1) } + let(:valid_description) { 'short description' } + let(:issuable) { build(:issue, description: description) } + + let(:error_message) do + format( + _('is too long (%{size}). The maximum size is %{max_size}.'), + size: ActiveSupport::NumberHelper.number_to_human_size(invalid_description.bytesize), + max_size: ActiveSupport::NumberHelper.number_to_human_size(::Issuable::DESCRIPTION_LENGTH_MAX) + ) + end - subject { issuable.validate(context) } + subject(:validate) { issuable.validate(context) } context 'when Issuable is a new record' do - it 'validates the maximum description length' do - subject - expect(issuable.errors[:description]).to eq(["is too long (maximum is #{::Issuable::DESCRIPTION_LENGTH_MAX} characters)"]) - end + let(:context) { :create } + + context 'when description exceeds the maximum size' do + let(:description) { invalid_description } - context 'on create' do - let(:context) { :create } + it 'adds a description too long error' do + validate - it 'does not validate the maximum description length' do - allow(issuable).to receive(:description_max_length_for_new_records_is_valid).and_call_original + expect(issuable.errors[:description]).to contain_exactly(error_message) + end + end - subject + context 'when description is within the allowed limits' do + let(:description) { valid_description } - expect(issuable).not_to have_received(:description_max_length_for_new_records_is_valid) + it 'does not add a validation error' do + validate + + expect(issuable.errors).not_to have_key(:description) end end end context 'when Issuable is an existing record' do + let(:context) { :update } + before do allow(issuable).to receive(:expire_etag_cache) # to skip the expire_etag_cache callback + issuable.description = existing_description issuable.save!(validate: false) + issuable.description = description + end + + context 'when record already had a valid description' do + let(:existing_description) { 'small difference so it triggers description_changed?' } + + context 'when new description exceeds the maximum size' do + let(:description) { invalid_description } + + it 'adds a description too long error' do + validate + + expect(issuable.errors[:description]).to contain_exactly(error_message) + end + end + + context 'when new description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(issuable.errors).not_to have_key(:description) + end + end end - it 'does not validate the maximum description length' do - subject - expect(issuable.errors).not_to have_key(:description) + context 'when record existed with an invalid description' do + let(:existing_description) { "#{invalid_description} small difference so it triggers description_changed?" } + + context 'when description is not changed' do + let(:description) { existing_description } + + it 'does not add a validation error' do + validate + + expect(issuable.errors).not_to have_key(:description) + end + end + + context 'when new description exceeds the maximum size' do + let(:description) { invalid_description } + + it 'allows updating descriptions that already existed above the limit' do + validate + + expect(issuable.errors).not_to have_key(:description) + end + end + + context 'when new description is within the allowed limits' do + let(:description) { valid_description } + + it 'does not add a validation error' do + validate + + expect(issuable.errors).not_to have_key(:description) + end + end end end end diff --git a/spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb b/spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb index aedbfe4deb3..9bfa4ace05c 100644 --- a/spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb @@ -32,8 +32,25 @@ RSpec.shared_examples 'sanitizable' do |factory, fields| subject { build(factory, attributes) } it 'is not valid', :aggregate_failures do + error = 'cannot contain escaped HTML entities' + + expect(subject).not_to be_valid + expect(subject.errors.details[field].flat_map(&:values)).to contain_exactly(error) + end + end + + context 'when it contains a path component' do + let_it_be(:input) do + 'main../../../../../../api/v4/projects/1/import_project_members/2' + end + + subject { build(factory, attributes) } + + it 'is not valid', :aggregate_failures do + error = 'cannot contain a path traversal component' + expect(subject).not_to be_valid - expect(subject.errors.details[field].flat_map(&:values)).to include('cannot contain escaped HTML entities') + expect(subject.errors.details[field].flat_map(&:values)).to contain_exactly(error) end end end -- cgit v1.2.1