diff options
-rw-r--r-- | .gitlab/ci/global.gitlab-ci.yml | 24 | ||||
-rw-r--r-- | .gitlab/ci/rails/shared.gitlab-ci.yml | 2 | ||||
-rw-r--r-- | .rubocop_todo/performance/redundant_split_regexp_argument.yml | 8 | ||||
-rw-r--r-- | GITALY_SERVER_VERSION | 2 | ||||
-rw-r--r-- | app/workers/bulk_import_worker.rb | 16 | ||||
-rw-r--r-- | app/workers/bulk_imports/pipeline_worker.rb | 13 | ||||
-rw-r--r-- | doc/user/application_security/dast/checks/22.1.md | 38 | ||||
-rw-r--r-- | doc/user/application_security/dast/checks/index.md | 8 | ||||
-rw-r--r-- | lib/bulk_imports/pipeline.rb | 1 | ||||
-rw-r--r-- | lib/file_size_validator.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/git/raw_diff_change.rb | 2 | ||||
-rw-r--r-- | lib/kramdown/converter/commonmark.rb | 2 | ||||
-rwxr-xr-x | scripts/db_tasks | 2 | ||||
-rw-r--r-- | spec/support/webmock.rb | 14 | ||||
-rw-r--r-- | spec/workers/bulk_import_worker_spec.rb | 25 | ||||
-rw-r--r-- | spec/workers/bulk_imports/pipeline_worker_spec.rb | 50 |
16 files changed, 147 insertions, 62 deletions
diff --git a/.gitlab/ci/global.gitlab-ci.yml b/.gitlab/ci/global.gitlab-ci.yml index 98ec3ce96a2..faae7ad2817 100644 --- a/.gitlab/ci/global.gitlab-ci.yml +++ b/.gitlab/ci/global.gitlab-ci.yml @@ -263,9 +263,13 @@ - name: redis:5.0-alpine - name: elasticsearch:7.17.6 command: ["elasticsearch", "-E", "discovery.type=single-node", "-E", "xpack.security.enabled=false"] + - name: registry.gitlab.com/gitlab-org/gitlab-build-images:zoekt-ci-image-1.0 + alias: zoekt-ci-image variables: POSTGRES_HOST_AUTH_METHOD: trust PG_VERSION: "11" + ZOEKT_INDEX_BASE_URL: http://zoekt-ci-image:6060 + ZOEKT_SEARCH_BASE_URL: http://zoekt-ci-image:6070 .use-pg12-es7-ee: services: @@ -274,9 +278,13 @@ - name: redis:6.0-alpine - name: elasticsearch:7.17.6 command: ["elasticsearch", "-E", "discovery.type=single-node", "-E", "xpack.security.enabled=false"] + - name: registry.gitlab.com/gitlab-org/gitlab-build-images:zoekt-ci-image-1.0 + alias: zoekt-ci-image variables: POSTGRES_HOST_AUTH_METHOD: trust PG_VERSION: "12" + ZOEKT_INDEX_BASE_URL: http://zoekt-ci-image:6060 + ZOEKT_SEARCH_BASE_URL: http://zoekt-ci-image:6070 .use-pg13-es7-ee: services: @@ -285,9 +293,13 @@ - name: redis:6.2-alpine - name: elasticsearch:7.17.6 command: ["elasticsearch", "-E", "discovery.type=single-node", "-E", "xpack.security.enabled=false"] + - name: registry.gitlab.com/gitlab-org/gitlab-build-images:zoekt-ci-image-1.0 + alias: zoekt-ci-image variables: POSTGRES_HOST_AUTH_METHOD: trust PG_VERSION: "13" + ZOEKT_INDEX_BASE_URL: http://zoekt-ci-image:6060 + ZOEKT_SEARCH_BASE_URL: http://zoekt-ci-image:6070 .use-pg12-es8-ee: services: @@ -295,11 +307,15 @@ command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"] - name: redis:6.0-alpine - name: elasticsearch:8.5.3 + - name: registry.gitlab.com/gitlab-org/gitlab-build-images:zoekt-ci-image-1.0 + alias: zoekt-ci-image variables: POSTGRES_HOST_AUTH_METHOD: trust PG_VERSION: "12" ES_SETTING_DISCOVERY_TYPE: "single-node" ES_SETTING_XPACK_SECURITY_ENABLED: "false" + ZOEKT_INDEX_BASE_URL: http://zoekt-ci-image:6060 + ZOEKT_SEARCH_BASE_URL: http://zoekt-ci-image:6070 .use-pg12-opensearch1-ee: services: @@ -309,9 +325,13 @@ - name: opensearchproject/opensearch:1.3.5 alias: elasticsearch command: ["bin/opensearch", "-E", "discovery.type=single-node", "-E", "plugins.security.disabled=true"] + - name: registry.gitlab.com/gitlab-org/gitlab-build-images:zoekt-ci-image-1.0 + alias: zoekt-ci-image variables: POSTGRES_HOST_AUTH_METHOD: trust PG_VERSION: "12" + ZOEKT_INDEX_BASE_URL: http://zoekt-ci-image:6060 + ZOEKT_SEARCH_BASE_URL: http://zoekt-ci-image:6070 .use-pg12-opensearch2-ee: services: @@ -321,9 +341,13 @@ - name: opensearchproject/opensearch:2.2.1 alias: elasticsearch command: ["bin/opensearch", "-E", "discovery.type=single-node", "-E", "plugins.security.disabled=true"] + - name: registry.gitlab.com/gitlab-org/gitlab-build-images:zoekt-ci-image-1.0 + alias: zoekt-ci-image variables: POSTGRES_HOST_AUTH_METHOD: trust PG_VERSION: "12" + ZOEKT_INDEX_BASE_URL: http://zoekt-ci-image:6060 + ZOEKT_SEARCH_BASE_URL: http://zoekt-ci-image:6070 .use-kaniko: image: diff --git a/.gitlab/ci/rails/shared.gitlab-ci.yml b/.gitlab/ci/rails/shared.gitlab-ci.yml index 3f9a43c1ced..4943f7c2e28 100644 --- a/.gitlab/ci/rails/shared.gitlab-ci.yml +++ b/.gitlab/ci/rails/shared.gitlab-ci.yml @@ -67,7 +67,7 @@ include: # spec/lib, yet background migration tests are also sitting there, # and they should run on their own jobs so we don't need to run them # in unit tests again. - - rspec_paralellized_job "--tag ~quarantine --tag ~zoekt --tag ~level:background_migration" + - rspec_paralellized_job "--tag ~quarantine --tag ~level:background_migration" allow_failure: exit_codes: !reference [.rspec-base, variables, SUCCESSFULLY_RETRIED_TEST_EXIT_CODE] diff --git a/.rubocop_todo/performance/redundant_split_regexp_argument.yml b/.rubocop_todo/performance/redundant_split_regexp_argument.yml deleted file mode 100644 index 0c0e12480d5..00000000000 --- a/.rubocop_todo/performance/redundant_split_regexp_argument.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -# Cop supports --autocorrect. -Performance/RedundantSplitRegexpArgument: - Details: grace period - Exclude: - - 'lib/file_size_validator.rb' - - 'lib/gitlab/git/raw_diff_change.rb' - - 'lib/kramdown/converter/commonmark.rb' diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 3a65378f961..fc6727dfc98 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -6eb7389db0d54ef9ba3622eb7d1412f71e9deb2f +78afcc347d19840f86d35967f35e1f7899080247 diff --git a/app/workers/bulk_import_worker.rb b/app/workers/bulk_import_worker.rb index d5eca86744e..6bce13c5ff0 100644 --- a/app/workers/bulk_import_worker.rb +++ b/app/workers/bulk_import_worker.rb @@ -4,6 +4,7 @@ class BulkImportWorker # rubocop:disable Scalability/IdempotentWorker include ApplicationWorker PERFORM_DELAY = 5.seconds + DEFAULT_BATCH_SIZE = 5 data_consistency :always feature_category :importers @@ -16,10 +17,11 @@ class BulkImportWorker # rubocop:disable Scalability/IdempotentWorker return if @bulk_import.finished? || @bulk_import.failed? return @bulk_import.fail_op! if all_entities_failed? return @bulk_import.finish! if all_entities_processed? && @bulk_import.started? + return re_enqueue if max_batch_size_exceeded? # Do not start more jobs if max allowed are already running @bulk_import.start! if @bulk_import.created? - created_entities.find_each do |entity| + created_entities.first(next_batch_size).each do |entity| BulkImports::CreatePipelineTrackersService.new(entity).execute! entity.start! @@ -58,4 +60,16 @@ class BulkImportWorker # rubocop:disable Scalability/IdempotentWorker def re_enqueue BulkImportWorker.perform_in(PERFORM_DELAY, @bulk_import.id) end + + def started_entities + entities.with_status(:started) + end + + def max_batch_size_exceeded? + started_entities.count >= DEFAULT_BATCH_SIZE + end + + def next_batch_size + [DEFAULT_BATCH_SIZE - started_entities.count, 0].max + end end diff --git a/app/workers/bulk_imports/pipeline_worker.rb b/app/workers/bulk_imports/pipeline_worker.rb index 62e85d38e61..7435a1f6a88 100644 --- a/app/workers/bulk_imports/pipeline_worker.rb +++ b/app/workers/bulk_imports/pipeline_worker.rb @@ -42,7 +42,6 @@ module BulkImports def run return skip_tracker if entity.failed? - raise(Pipeline::ExpiredError, 'Pipeline timeout') if job_timeout? raise(Pipeline::FailedError, "Export from source instance failed: #{export_status.error}") if export_failed? raise(Pipeline::ExpiredError, 'Empty export status on source instance') if empty_export_timeout? @@ -103,14 +102,8 @@ module BulkImports pipeline_tracker.file_extraction_pipeline? end - def job_timeout? - return false unless file_extraction_pipeline? - - time_since_entity_created > Pipeline::NDJSON_EXPORT_TIMEOUT - end - def empty_export_timeout? - export_empty? && time_since_entity_created > Pipeline::EMPTY_EXPORT_STATUS_TIMEOUT + export_empty? && time_since_tracker_created > Pipeline::EMPTY_EXPORT_STATUS_TIMEOUT end def export_failed? @@ -167,8 +160,8 @@ module BulkImports logger.error(structured_payload(payload)) end - def time_since_entity_created - Time.zone.now - entity.created_at + def time_since_tracker_created + Time.zone.now - (pipeline_tracker.created_at || entity.created_at) end def lease_timeout diff --git a/doc/user/application_security/dast/checks/22.1.md b/doc/user/application_security/dast/checks/22.1.md new file mode 100644 index 00000000000..60a73b4248b --- /dev/null +++ b/doc/user/application_security/dast/checks/22.1.md @@ -0,0 +1,38 @@ +--- +stage: Secure +group: Dynamic Analysis +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# Improper limitation of a pathname to a restricted directory (Path traversal) + +## Description + +The vulnerability can be exploited by inserting a payload into a +parameter on the URL endpoint which allows for reading arbitrary files. +This could be used to read sensitive files, access other users data, or aid in +exploitation to gain further system access. + +## Remediation + +User input should never be used in constructing paths or files for interacting +with the filesystem. This includes filenames supplied by user uploads or downloads. + +If possible, consider hashing the filenames and reference the hashed filenames in +a database or datastore instead of directly attempting to access filenames provided +by users or other system components. + +In the rare cases that the application must work with filenames, use the language +provided functionality to extract only the filename part of the supplied value. +Never attempt to use the path or directory information that comes from user input. + +## Details + +| ID | Aggregated | CWE | Type | Risk | +|:---|:--------|:--------|:--------|:--------| +| 22.1 | false | 22 | Active | high | + +## Links + +- [OWASP](https://owasp.org/www-community/attacks/Path_Traversal) +- [CWE](https://cwe.mitre.org/data/definitions/22.html) diff --git a/doc/user/application_security/dast/checks/index.md b/doc/user/application_security/dast/checks/index.md index 0f9e78d1817..bafe426ca43 100644 --- a/doc/user/application_security/dast/checks/index.md +++ b/doc/user/application_security/dast/checks/index.md @@ -8,6 +8,8 @@ info: To determine the technical writer assigned to the Stage/Group associated w The [DAST browser-based crawler](../browser_based.md) provides a number of vulnerability checks that are used to scan for vulnerabilities in the site under test. +## Passive Checks + | ID | Check | Severity | Type | |:---|:------|:---------|:-----| | [1004.1](1004.1.md) | Sensitive cookie without HttpOnly attribute | Low | Passive | @@ -160,3 +162,9 @@ The [DAST browser-based crawler](../browser_based.md) provides a number of vulne | [798.128](798.128.md) | Exposure of confidential secret or token Zendesk Secret Key | High | Passive | | [829.1](829.1.md) | Inclusion of Functionality from Untrusted Control Sphere | Low | Passive | | [829.2](829.2.md) | Invalid Sub-Resource Integrity values detected | Medium | Passive | + +## Active Checks + +| ID | Check | Severity | Type | +|:---|:------|:---------|:-----| +| [22.1](22.1.md) | Improper limitation of a pathname to a restricted directory (Path traversal) | High | Active | diff --git a/lib/bulk_imports/pipeline.rb b/lib/bulk_imports/pipeline.rb index 681d6e9aad6..e65232a5734 100644 --- a/lib/bulk_imports/pipeline.rb +++ b/lib/bulk_imports/pipeline.rb @@ -12,7 +12,6 @@ module BulkImports FailedError = Class.new(StandardError) CACHE_KEY_EXPIRATION = 2.hours - NDJSON_EXPORT_TIMEOUT = 90.minutes EMPTY_EXPORT_STATUS_TIMEOUT = 5.minutes def initialize(context) diff --git a/lib/file_size_validator.rb b/lib/file_size_validator.rb index ac3210b4e98..8ac5f2f6b54 100644 --- a/lib/file_size_validator.rb +++ b/lib/file_size_validator.rb @@ -4,7 +4,7 @@ class FileSizeValidator < ActiveModel::EachValidator MESSAGES = { is: :wrong_size, minimum: :size_too_small, maximum: :size_too_big }.freeze CHECKS = { is: :==, minimum: :>=, maximum: :<= }.freeze - DEFAULT_TOKENIZER = -> (value) { value.split(//) }.freeze + DEFAULT_TOKENIZER = -> (value) { value.split("") }.freeze RESERVED_OPTIONS = [:minimum, :maximum, :within, :is, :tokenizer, :too_short, :too_long].freeze def initialize(options) diff --git a/lib/gitlab/git/raw_diff_change.rb b/lib/gitlab/git/raw_diff_change.rb index 9a41f04a4db..c0121311605 100644 --- a/lib/gitlab/git/raw_diff_change.rb +++ b/lib/gitlab/git/raw_diff_change.rb @@ -38,7 +38,7 @@ module Gitlab def extract_paths(file_path) case operation when :copied, :renamed - file_path.split(/\t/) + file_path.split("\t") when :deleted [file_path, nil] when :added diff --git a/lib/kramdown/converter/commonmark.rb b/lib/kramdown/converter/commonmark.rb index a903d541d81..0055558989d 100644 --- a/lib/kramdown/converter/commonmark.rb +++ b/lib/kramdown/converter/commonmark.rb @@ -34,7 +34,7 @@ module Kramdown def convert_codeblock(el, _opts) # Although tildes are supported in CommonMark, backticks are more common "```#{el.options[:lang]}\n" + - el.value.split(/\n/).map { |l| l.empty? ? "" : "#{l}" }.join("\n") + + el.value.split("\n").map { |l| l.empty? ? "" : "#{l}" }.join("\n") + "\n```\n\n" end diff --git a/scripts/db_tasks b/scripts/db_tasks index c3b7bb84355..36040877abf 100755 --- a/scripts/db_tasks +++ b/scripts/db_tasks @@ -9,7 +9,7 @@ database_config = YAML.load_file(File.join(File.expand_path('..', __dir__), 'con task = ARGV.shift raise ArgumentError, 'You need to pass a task name!' unless task -task = "${task}:main" unless database_config.one? +task = "#{task}:main" unless database_config.one? cmd = ['bundle', 'exec', 'rake', task, *ARGV] puts "Running: `#{cmd.join(' ')}`" diff --git a/spec/support/webmock.rb b/spec/support/webmock.rb index b9bd3f82f65..171c7ace2d2 100644 --- a/spec/support/webmock.rb +++ b/spec/support/webmock.rb @@ -9,12 +9,26 @@ def webmock_allowed_hosts hosts << URI.parse(ENV['ELASTIC_URL']).host end + if ENV.key?('ZOEKT_INDEX_BASE_URL') + hosts.concat(allowed_host_and_ip(ENV['ZOEKT_INDEX_BASE_URL'])) + end + + if ENV.key?('ZOEKT_SEARCH_BASE_URL') + hosts.concat(allowed_host_and_ip(ENV['ZOEKT_SEARCH_BASE_URL'])) + end + if Gitlab.config.webpack&.dev_server&.enabled hosts << Gitlab.config.webpack.dev_server.host end end.compact.uniq end +def allowed_host_and_ip(url) + host = URI.parse(url).host + ip_address = Addrinfo.ip(host).ip_address + [host, ip_address] +end + def with_net_connect_allowed WebMock.allow_net_connect! yield diff --git a/spec/workers/bulk_import_worker_spec.rb b/spec/workers/bulk_import_worker_spec.rb index 61c33f123fa..ec8550bb3bc 100644 --- a/spec/workers/bulk_import_worker_spec.rb +++ b/spec/workers/bulk_import_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkImportWorker do +RSpec.describe BulkImportWorker, feature_category: :importers do describe '#perform' do context 'when no bulk import is found' do it 'does nothing' do @@ -56,6 +56,19 @@ RSpec.describe BulkImportWorker do end end + context 'when maximum allowed number of import entities in progress' do + it 'reenqueues itself' do + bulk_import = create(:bulk_import, :started) + create(:bulk_import_entity, :created, bulk_import: bulk_import) + (described_class::DEFAULT_BATCH_SIZE + 1).times { |_| create(:bulk_import_entity, :started, bulk_import: bulk_import) } + + expect(described_class).to receive(:perform_in).with(described_class::PERFORM_DELAY, bulk_import.id) + expect(BulkImports::ExportRequestWorker).not_to receive(:perform_async) + + subject.perform(bulk_import.id) + end + end + context 'when bulk import is created' do it 'marks bulk import as started' do bulk_import = create(:bulk_import, :created) @@ -82,16 +95,20 @@ RSpec.describe BulkImportWorker do context 'when there are created entities to process' do let_it_be(:bulk_import) { create(:bulk_import, :created) } - it 'marks all entities as started, enqueues EntityWorker, ExportRequestWorker and reenqueues' do + before do + stub_const("#{described_class}::DEFAULT_BATCH_SIZE", 1) + end + + it 'marks a batch of entities as started, enqueues EntityWorker, ExportRequestWorker and reenqueues' do create(:bulk_import_entity, :created, bulk_import: bulk_import) create(:bulk_import_entity, :created, bulk_import: bulk_import) expect(described_class).to receive(:perform_in).with(described_class::PERFORM_DELAY, bulk_import.id) - expect(BulkImports::ExportRequestWorker).to receive(:perform_async).twice + expect(BulkImports::ExportRequestWorker).to receive(:perform_async).once subject.perform(bulk_import.id) - expect(bulk_import.entities.map(&:status_name)).to contain_exactly(:started, :started) + expect(bulk_import.entities.map(&:status_name)).to contain_exactly(:created, :started) end context 'when there are project entities to process' do diff --git a/spec/workers/bulk_imports/pipeline_worker_spec.rb b/spec/workers/bulk_imports/pipeline_worker_spec.rb index 03ec6267ca8..10c63606f81 100644 --- a/spec/workers/bulk_imports/pipeline_worker_spec.rb +++ b/spec/workers/bulk_imports/pipeline_worker_spec.rb @@ -433,11 +433,11 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do allow(status).to receive(:failed?).and_return(false) end - entity.update!(created_at: entity_created_at) + pipeline_tracker.update!(created_at: created_at) end context 'when timeout is not reached' do - let(:entity_created_at) { 1.minute.ago } + let(:created_at) { 1.minute.ago } it 'reenqueues pipeline worker' do expect(described_class) @@ -455,8 +455,8 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do end end - context 'when timeout is reached' do - let(:entity_created_at) { 10.minutes.ago } + context 'when empty export timeout is reached' do + let(:created_at) { 10.minutes.ago } it 'marks as failed and logs the error' do expect_next_instance_of(Gitlab::Import::Logger) do |logger| @@ -485,39 +485,25 @@ RSpec.describe BulkImports::PipelineWorker, feature_category: :importers do expect(pipeline_tracker.reload.status_name).to eq(:failed) end end - end - context 'when job reaches timeout' do - it 'marks as failed and logs the error' do - old_created_at = entity.created_at - entity.update!(created_at: (BulkImports::Pipeline::NDJSON_EXPORT_TIMEOUT + 1.hour).ago) + context 'when tracker created_at is nil' do + let(:created_at) { nil } - expect_next_instance_of(Gitlab::Import::Logger) do |logger| - expect(logger) - .to receive(:error) - .with( - hash_including( - 'pipeline_name' => 'NdjsonPipeline', - 'bulk_import_entity_id' => entity.id, - 'bulk_import_id' => entity.bulk_import_id, - 'bulk_import_entity_type' => entity.source_type, - 'source_full_path' => entity.source_full_path, - 'class' => 'BulkImports::PipelineWorker', - 'exception.backtrace' => anything, - 'exception.class' => 'BulkImports::Pipeline::ExpiredError', - 'exception.message' => 'Pipeline timeout', - 'importer' => 'gitlab_migration', - 'message' => 'Pipeline failed', - 'source_version' => entity.bulk_import.source_version_info.to_s - ) - ) - end + it 'falls back to entity created_at' do + entity.update!(created_at: 10.minutes.ago) - subject.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + expect_next_instance_of(Gitlab::Import::Logger) do |logger| + expect(logger) + .to receive(:error) + .with( + hash_including('exception.message' => 'Empty export status on source instance') + ) + end - expect(pipeline_tracker.reload.status_name).to eq(:failed) + subject.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) - entity.update!(created_at: old_created_at) + expect(pipeline_tracker.reload.status_name).to eq(:failed) + end end end |