From 14c3ede71a72abb5d88c9f08e442fefd7f5cb230 Mon Sep 17 00:00:00 2001
From: Nick Malcolm <nmalcolm@gitlab.com>
Date: Wed, 21 Dec 2022 19:51:21 +0000
Subject: [PATCH] Clearly test the expectations around MAX_VERSION_LENGTH
In response to MR feedback: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107335#note_1216705882
---
lib/gitlab/import_export/version_checker.rb | 2 +-
lib/gitlab/version_info.rb | 5 ++++-
spec/lib/gitlab/import_export/version_checker_spec.rb | 2 +-
spec/lib/gitlab/version_info_spec.rb | 8 ++++++++
4 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/lib/gitlab/import_export/version_checker.rb b/lib/gitlab/import_export/version_checker.rb
index 5ec9db00d0a68..ad071a4cbd7a5 100644
--- a/lib/gitlab/import_export/version_checker.rb
+++ b/lib/gitlab/import_export/version_checker.rb
@@ -34,7 +34,7 @@ def verify_version!(version)
end
def different_version?(version)
- Gem::Version.new(version) != Gem::Version.new(Gitlab::ImportExport.version)
+ Gitlab::VersionInfo.parse(version) != Gitlab::VersionInfo.parse(Gitlab::ImportExport.version)
rescue StandardError => e
Gitlab::Import::Logger.error(
message: 'Import error',
diff --git a/lib/gitlab/version_info.rb b/lib/gitlab/version_info.rb
index 61de003c28d37..0351c9b30b3ad 100644
--- a/lib/gitlab/version_info.rb
+++ b/lib/gitlab/version_info.rb
@@ -7,11 +7,14 @@ class VersionInfo
attr_reader :major, :minor, :patch
VERSION_REGEX = /(\d+)\.(\d+)\.(\d+)/.freeze
+ # To mitigate ReDoS, limit the length of the version string we're
+ # willing to check
+ MAX_VERSION_LENGTH = 128
def self.parse(str, parse_suffix: false)
if str.is_a?(self)
str
- elsif str && m = str.match(VERSION_REGEX)
+ elsif str && str.length <= MAX_VERSION_LENGTH && m = str.match(VERSION_REGEX)
VersionInfo.new(m[1].to_i, m[2].to_i, m[3].to_i, parse_suffix ? m.post_match : nil)
else
VersionInfo.new
diff --git a/spec/lib/gitlab/import_export/version_checker_spec.rb b/spec/lib/gitlab/import_export/version_checker_spec.rb
index 14c62edb7868b..b3730d85f13a3 100644
--- a/spec/lib/gitlab/import_export/version_checker_spec.rb
+++ b/spec/lib/gitlab/import_export/version_checker_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Gitlab::ImportExport::VersionChecker do
+RSpec.describe Gitlab::ImportExport::VersionChecker, feature_category: :import do
include ImportExport::CommonUtil
let!(:shared) { Gitlab::ImportExport::Shared.new(nil) }
diff --git a/spec/lib/gitlab/version_info_spec.rb b/spec/lib/gitlab/version_info_spec.rb
index 078f952afad78..99c7a762392b3 100644
--- a/spec/lib/gitlab/version_info_spec.rb
+++ b/spec/lib/gitlab/version_info_spec.rb
@@ -92,6 +92,8 @@
it { expect(described_class.parse("1.0.0-rc1-ee")).to eq(@v1_0_0) }
it { expect(described_class.parse("git 1.0.0b1")).to eq(@v1_0_0) }
it { expect(described_class.parse("git 1.0b1")).not_to be_valid }
+ it { expect(described_class.parse("1.1.#{'1' * described_class::MAX_VERSION_LENGTH}")).not_to be_valid }
+ it { expect(described_class.parse(nil)).not_to be_valid }
context 'with parse_suffix: true' do
let(:versions) do
@@ -182,4 +184,10 @@
it { expect(@v1_0_1.without_patch).to eq(@v1_0_0) }
it { expect(@v1_0_1_rc1.without_patch).to eq(@v1_0_0) }
end
+
+ describe 'MAX_VERSION_LENGTH' do
+ subject { described_class::MAX_VERSION_LENGTH }
+
+ it { is_expected.to eq(128) }
+ end
end
--
GitLab