summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2023-01-30 09:13:00 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2023-01-30 09:13:00 +0000
commite92925533667e147ff34cf1e9b8af21680c8c7d4 (patch)
tree1594de73938c3015737864b667e43b5e9650c9fb
parentc3e54801bb461b6d53c48e3194f87cb5ebf3f5ba (diff)
downloadgitlab-ce-e92925533667e147ff34cf1e9b8af21680c8c7d4.tar.gz
Add latest changes from gitlab-org/security/gitlab@15-8-stable-ee
-rw-r--r--app/models/concerns/issuable.rb19
-rw-r--r--app/models/concerns/sanitizable.rb9
-rw-r--r--app/models/namespace_setting.rb13
-rw-r--r--app/services/packages/helm/extract_file_metadata_service.rb5
-rw-r--r--lib/gitlab/ci/artifact_file_reader.rb23
-rw-r--r--lib/gitlab/utils.rb3
-rw-r--r--lib/safe_zip/entry.rb10
-rw-r--r--lib/safe_zip/extract.rb1
-rw-r--r--lib/safe_zip/extract_params.rb24
-rw-r--r--spec/fixtures/packages/helm/corrupted_chart.tgzbin0 -> 2084191 bytes
-rw-r--r--spec/fixtures/safe_zip/invalid-unexpected-large.zipbin0 -> 376 bytes
-rw-r--r--spec/fixtures/safe_zip/valid-symlinks-first.zipbin528 -> 1297 bytes
-rw-r--r--spec/lib/gitlab/ci/artifact_file_reader_spec.rb124
-rw-r--r--spec/lib/safe_zip/entry_spec.rb35
-rw-r--r--spec/lib/safe_zip/extract_params_spec.rb35
-rw-r--r--spec/lib/safe_zip/extract_spec.rb55
-rw-r--r--spec/models/concerns/issuable_spec.rb1
-rw-r--r--spec/models/concerns/sanitizable_spec.rb53
-rw-r--r--spec/models/namespace_setting_spec.rb6
-rw-r--r--spec/services/packages/helm/extract_file_metadata_service_spec.rb13
-rw-r--r--spec/support/shared_examples/models/concerns/issuable_shared_examples.rb103
-rw-r--r--spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb19
22 files changed, 440 insertions, 111 deletions
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
--- /dev/null
+++ b/spec/fixtures/packages/helm/corrupted_chart.tgz
Binary files 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
--- /dev/null
+++ b/spec/fixtures/safe_zip/invalid-unexpected-large.zip
Binary files 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
--- a/spec/fixtures/safe_zip/valid-symlinks-first.zip
+++ b/spec/fixtures/safe_zip/valid-symlinks-first.zip
Binary files 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&lt;script&gt;alert(1)&lt;/script&gt;'
+ 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: (<unknown>): 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