Merge branch 'security-10io-validate-nuget-package-name' into 'master'

Validate nuget package names

See merge request gitlab-org/security/gitlab!1039

app/models/packages/package.rb

@@ -37,6 +37,7 @@ class Packages::Package < ApplicationRecord
   validate :package_already_taken, if: :npm?
   validates :name, format: { with: Gitlab::Regex.conan_recipe_component_regex }, if: :conan?
   validates :name, format: { with: Gitlab::Regex.generic_package_name_regex }, if: :generic?
+  validates :name, format: { with: Gitlab::Regex.nuget_package_name_regex }, if: :nuget?
   validates :version, format: { with: Gitlab::Regex.nuget_version_regex }, if: :nuget?
   validates :version, format: { with: Gitlab::Regex.conan_recipe_component_regex }, if: :conan?
   validates :version, format: { with: Gitlab::Regex.maven_version_regex }, if: -> { version? && maven? }

app/services/packages/nuget/update_package_from_metadata_service.rb

@@ -32,6 +32,8 @@ module Packages
             )
           end
         end
+      rescue ActiveRecord::RecordInvalid => e
+        raise InvalidMetadataError.new(e.message)
       end
 
       private

changelogs/unreleased/security-10io-validate-nuget-package-name.yml

+---
+title: Validate nuget package names
+merge_request:
+author:
+type: security

lib/gitlab/regex.rb

@@ -50,6 +50,10 @@ module Gitlab
         maven_app_name_regex
       end
 
+      def nuget_package_name_regex
+        @nuget_package_name_regex ||= %r{\A[-+\.\_a-zA-Z0-9]+\z}.freeze
+      end
+
       def nuget_version_regex
         @nuget_version_regex ||= /
           \A#{_semver_major_minor_patch_regex}(\.\d*)?#{_semver_prerelease_build_regex}\z

spec/lib/gitlab/regex_spec.rb

@@ -377,6 +377,21 @@ RSpec.describe Gitlab::Regex do
     it { is_expected.not_to match('%2e%2e%2f1.2.3') }
   end
 
+  describe '.nuget_package_name_regex' do
+    subject { described_class.nuget_package_name_regex }
+
+    it { is_expected.to match('My.Package') }
+    it { is_expected.to match('My.Package.Mvc') }
+    it { is_expected.to match('MyPackage') }
+    it { is_expected.to match('My.23.Package') }
+    it { is_expected.to match('My23Package') }
+    it { is_expected.to match('runtime.my-test64.runtime.package.Mvc') }
+    it { is_expected.to match('my_package') }
+    it { is_expected.not_to match('My/package') }
+    it { is_expected.not_to match('../../../my_package') }
+    it { is_expected.not_to match('%2e%2e%2fmy_package') }
+  end
+
   describe '.pypi_version_regex' do
     subject { described_class.pypi_version_regex }
 

spec/models/packages/package_spec.rb

@@ -122,6 +122,21 @@ RSpec.describe Packages::Package, type: :model do
         it { is_expected.not_to allow_value('my file name').for(:name) }
         it { is_expected.not_to allow_value('!!().for(:name)().for(:name)').for(:name) }
       end
+
+      context 'nuget package' do
+        subject { build_stubbed(:nuget_package) }
+
+        it { is_expected.to allow_value('My.Package').for(:name) }
+        it { is_expected.to allow_value('My.Package.Mvc').for(:name) }
+        it { is_expected.to allow_value('MyPackage').for(:name) }
+        it { is_expected.to allow_value('My.23.Package').for(:name) }
+        it { is_expected.to allow_value('My23Package').for(:name) }
+        it { is_expected.to allow_value('runtime.my-test64.runtime.package.Mvc').for(:name) }
+        it { is_expected.to allow_value('my_package').for(:name) }
+        it { is_expected.not_to allow_value('My/package').for(:name) }
+        it { is_expected.not_to allow_value('../../../my_package').for(:name) }
+        it { is_expected.not_to allow_value('%2e%2e%2fmy_package').for(:name) }
+      end
     end
 
     describe '#version' do

spec/services/packages/nuget/update_package_from_metadata_service_spec.rb

@@ -198,24 +198,26 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_
       it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError
     end
 
-    context 'with package file with a blank package name' do
-      before do
-        allow(service).to receive(:package_name).and_return('')
-      end
+    context 'with an invalid package name' do
+      invalid_names = [
+        '',
+        'My/package',
+        '../../../my_package',
+        '%2e%2e%2fmy_package'
+      ]
 
-      it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
-    end
+      invalid_names.each do |invalid_name|
+        before do
+          allow(service).to receive(:package_name).and_return(invalid_name)
+        end
 
-    context 'with package file with a blank package version' do
-      before do
-        allow(service).to receive(:package_version).and_return('')
+        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
       end
-
-      it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
     end
 
     context 'with an invalid package version' do
       invalid_versions = [
+        '',
         '555',
         '1.2',
         '1./2.3',
@@ -224,13 +226,11 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_
       ]
 
       invalid_versions.each do |invalid_version|
-        it "raises an error for version #{invalid_version}" do
+        before do
           allow(service).to receive(:package_version).and_return(invalid_version)
-
-          expect { subject }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Version is invalid')
-          expect(package_file.file_name).not_to include(invalid_version)
-          expect(package_file.file.file.path).not_to include(invalid_version)
         end
+
+        it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError
       end
     end
   end

spec/workers/packages/nuget/extraction_worker_spec.rb

@@ -13,6 +13,18 @@ RSpec.describe Packages::Nuget::ExtractionWorker, type: :worker do
 
     subject { described_class.new.perform(package_file_id) }
 
+    shared_examples 'handling the metadata error' do |exception_class: ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError|
+      it 'removes the package and the package file' do
+        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
+          instance_of(exception_class),
+          project_id: package.project_id
+        )
+        expect { subject }
+          .to change { Packages::Package.count }.by(-1)
+          .and change { Packages::PackageFile.count }.by(-1)
+      end
+    end
+
     context 'with valid package file' do
       it 'updates package and package file' do
         expect { subject }
@@ -48,46 +60,46 @@ RSpec.describe Packages::Nuget::ExtractionWorker, type: :worker do
         allow_any_instance_of(Zip::File).to receive(:glob).and_return([])
       end
 
-      it 'removes the package and the package file' do
-        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
-          instance_of(::Packages::Nuget::MetadataExtractionService::ExtractionError),
-          project_id: package.project_id
-        )
-        expect { subject }
-          .to change { Packages::Package.count }.by(-1)
-          .and change { Packages::PackageFile.count }.by(-1)
-      end
+      it_behaves_like 'handling the metadata error', exception_class: ::Packages::Nuget::MetadataExtractionService::ExtractionError
     end
 
-    context 'with package file with a blank package name' do
-      before do
-        allow_any_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService).to receive(:package_name).and_return('')
-      end
+    context 'with package with an invalid package name' do
+      invalid_names = [
+        '',
+        'My/package',
+        '../../../my_package',
+        '%2e%2e%2fmy_package'
+      ]
 
-      it 'removes the package and the package file' do
-        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
-          instance_of(::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError),
-          project_id: package.project_id
-        )
-        expect { subject }
-          .to change { Packages::Package.count }.by(-1)
-          .and change { Packages::PackageFile.count }.by(-1)
+      invalid_names.each do |invalid_name|
+        before do
+          allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service|
+            allow(service).to receive(:package_name).and_return(invalid_name)
+          end
+        end
+
+        it_behaves_like 'handling the metadata error'
       end
     end
 
-    context 'with package file with a blank package version' do
-      before do
-        allow_any_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService).to receive(:package_version).and_return('')
-      end
+    context 'with package with an invalid package version' do
+      invalid_versions = [
+        '',
+        '555',
+        '1.2',
+        '1./2.3',
+        '../../../../../1.2.3',
+        '%2e%2e%2f1.2.3'
+      ]
 
-      it 'removes the package and the package file' do
-        expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
-          instance_of(::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError),
-          project_id: package.project_id
-        )
-        expect { subject }
-          .to change { Packages::Package.count }.by(-1)
-          .and change { Packages::PackageFile.count }.by(-1)
+      invalid_versions.each do |invalid_version|
+        before do
+          allow_next_instance_of(::Packages::Nuget::UpdatePackageFromMetadataService) do |service|
+            allow(service).to receive(:package_version).and_return(invalid_version)
+          end
+        end
+
+        it_behaves_like 'handling the metadata error'
       end
     end
   end