Merge branch 'security-path-traversal-npm-package-registry' into 'master'

Validate NPM package versions to be SemVer compliant

Closes #96

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

ee/app/models/packages/package.rb

@@ -29,6 +29,7 @@ class Packages::Package < ApplicationRecord
   validate :valid_conan_package_recipe, if: :conan?
   validate :valid_npm_package_name, if: :npm?
   validate :package_already_taken, if: :npm?
+  validates :version, format: { with: Gitlab::Regex.semver_regex }, if: :npm?
 
   enum package_type: { maven: 1, npm: 2, conan: 3, nuget: 4 }
 

ee/changelogs/unreleased/security-path-traversal-npm-package-registry.yml

+---
+title: Add NPM package versions SemVer validation
+merge_request:
+author:
+type: security

ee/lib/ee/gitlab/regex.rb

@@ -43,6 +43,11 @@ module EE
           maven_app_name_regex
         end
 
+        def semver_regex
+          # see the official regex: https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
+          @semver_regex ||= %r{\A(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?\z}.freeze
+        end
+
         def feature_flag_regex
           /\A[a-z]([-_a-z0-9]*[a-z0-9])?\z/
         end

ee/spec/lib/gitlab/regex_spec.rb

@@ -103,4 +103,17 @@ describe Gitlab::Regex do
     it { is_expected.not_to match('my package name') }
     it { is_expected.not_to match('!!()()') }
   end
+
+  describe '.semver_regex' do
+    subject { described_class.semver_regex }
+
+    it { is_expected.to match('1.2.3') }
+    it { is_expected.to match('1.2.3-beta') }
+    it { is_expected.to match('1.2.3-alpha.3') }
+    it { is_expected.not_to match('1') }
+    it { is_expected.not_to match('1.2') }
+    it { is_expected.not_to match('1./2.3') }
+    it { is_expected.not_to match('../../../../../1.2.3') }
+    it { is_expected.not_to match('%2e%2e%2f1.2.3') }
+  end
 end

ee/spec/models/packages/package_spec.rb

@@ -80,6 +80,21 @@ RSpec.describe Packages::Package, type: :model do
       it { is_expected.not_to allow_value("my(dom$$$ain)com.my-app").for(:name) }
     end
 
+    describe '#version' do
+      context 'npm package' do
+        subject { create(:npm_package) }
+
+        it { is_expected.to allow_value('1.2.3').for(:version) }
+        it { is_expected.to allow_value('1.2.3-beta').for(:version) }
+        it { is_expected.to allow_value('1.2.3-alpha.3').for(:version) }
+        it { is_expected.not_to allow_value('1').for(:version) }
+        it { is_expected.not_to allow_value('1.2').for(:version) }
+        it { is_expected.not_to allow_value('1./2.3').for(:version) }
+        it { is_expected.not_to allow_value('../../../../../1.2.3').for(:version) }
+        it { is_expected.not_to allow_value('%2e%2e%2f1.2.3').for(:version) }
+      end
+    end
+
     describe '#package_already_taken' do
       context 'npm package' do
         let!(:package) { create(:npm_package) }
@@ -173,7 +188,7 @@ RSpec.describe Packages::Package, type: :model do
     end
 
     describe '.has_version' do
-      let!(:package4) { create(:npm_package, version: nil) }
+      let!(:package4) { create(:nuget_package, version: nil) }
 
       subject { described_class.has_version }
 

ee/spec/requests/api/npm_packages_spec.rb

@@ -246,21 +246,25 @@ describe API::NpmPackages do
   end
 
   describe 'PUT /api/v4/projects/:id/packages/npm/:package_name' do
+    RSpec.shared_examples 'handling invalid record with 400 error' do
+      it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do
+        expect { upload_package_with_token(package_name, params) }
+          .not_to change { project.packages.count }
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
+
     context 'when params are correct' do
       context 'invalid package record' do
         context 'unscoped package' do
           let(:package_name) { 'my_unscoped_package' }
-          let(:params) { upload_params(package_name) }
+          let(:params) { upload_params(package_name: package_name) }
 
-          it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do
-            expect { upload_package_with_token(package_name, params) }
-              .not_to change { project.packages.count }
-
-            expect(response).to have_gitlab_http_status(:bad_request)
-          end
+          it_behaves_like 'handling invalid record with 400 error'
 
           context 'with empty versions' do
-            let(:params) { upload_params(package_name).merge!(versions: {}) }
+            let(:params) { upload_params(package_name: package_name).merge!(versions: {}) }
 
             it 'throws a 400 error' do
               expect { upload_package_with_token(package_name, params) }
@@ -273,20 +277,37 @@ describe API::NpmPackages do
 
         context 'invalid package name' do
           let(:package_name) { "@#{group.path}/my_inv@@lid_package_name" }
-          let(:params) { upload_params(package_name) }
+          let(:params) { upload_params(package_name: package_name) }
 
-          it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do
-            expect { upload_package_with_token(package_name, params) }
-              .not_to change { project.packages.count }
+          it_behaves_like 'handling invalid record with 400 error'
+        end
+
+        context 'invalid package version' do
+          using RSpec::Parameterized::TableSyntax
+
+          let(:package_name) { "@#{group.path}/my_package_name" }
+
+          where(:version) do
+            [
+              '1',
+              '1.2',
+              '1./2.3',
+              '../../../../../1.2.3',
+              '%2e%2e%2f1.2.3'
+            ]
+          end
+
+          with_them do
+            let(:params) { upload_params(package_name: package_name, package_version: version) }
 
-            expect(response).to have_gitlab_http_status(:bad_request)
+            it_behaves_like 'handling invalid record with 400 error'
           end
         end
       end
 
       context 'scoped package' do
         let(:package_name) { "@#{group.path}/my_package_name" }
-        let(:params) { upload_params(package_name) }
+        let(:params) { upload_params(package_name: package_name) }
 
         context 'with access token' do
           subject { upload_package_with_token(package_name, params) }
@@ -335,7 +356,7 @@ describe API::NpmPackages do
 
       context 'package creation fails' do
         let(:package_name) { "@#{group.path}/my_package_name" }
-        let(:params) { upload_params(package_name) }
+        let(:params) { upload_params(package_name: package_name) }
 
         it 'returns an error if the package already exists' do
           create(:npm_package, project: project, version: '1.0.1', name: "@#{group.path}/my_package_name")
@@ -348,7 +369,7 @@ describe API::NpmPackages do
 
       context 'with dependencies' do
         let(:package_name) { "@#{group.path}/my_package_name" }
-        let(:params) { upload_params(package_name, 'npm/payload_with_duplicated_packages.json') }
+        let(:params) { upload_params(package_name: package_name, file: 'npm/payload_with_duplicated_packages.json') }
 
         it 'creates npm package with file and dependencies' do
           expect { upload_package_with_token(package_name, params) }
@@ -363,7 +384,7 @@ describe API::NpmPackages do
         context 'with existing dependencies' do
           before do
             name = "@#{group.path}/existing_package"
-            upload_package_with_token(name, upload_params(name, 'npm/payload_with_duplicated_packages.json'))
+            upload_package_with_token(name, upload_params(package_name: name, file: 'npm/payload_with_duplicated_packages.json'))
           end
 
           it 'reuses them' do
@@ -389,10 +410,11 @@ describe API::NpmPackages do
       upload_package(package_name, params.merge(job_token: job.token))
     end
 
-    def upload_params(package_name, file = 'npm/payload.json')
+    def upload_params(package_name:, package_version: '1.0.1', file: 'npm/payload.json')
       JSON.parse(
         fixture_file(file, dir: 'ee')
-          .gsub('@root/npm-test', package_name))
+          .gsub('@root/npm-test', package_name)
+          .gsub('1.0.1', package_version))
     end
   end
 

ee/spec/services/packages/npm/create_package_service_spec.rb

@@ -5,7 +5,7 @@ describe Packages::Npm::CreatePackageService do
   let(:namespace) {create(:namespace)}
   let(:project) { create(:project, namespace: namespace) }
   let(:user) { create(:user) }
-  let(:version) { '1.0.1'.freeze }
+  let(:version) { '1.0.1' }
 
   let(:params) do
     JSON.parse(
@@ -37,7 +37,6 @@ describe Packages::Npm::CreatePackageService do
       expect(package.version).to eq(version)
     end
 
-    it { is_expected.to be_valid }
     it { expect(subject.name).to eq(package_name) }
     it { expect(subject.version).to eq(version) }
   end
@@ -77,5 +76,23 @@ describe Packages::Npm::CreatePackageService do
       it { expect(subject[:http_status]).to eq 400 }
       it { expect(subject[:message]).to eq 'Version is empty.' }
     end
+
+    context 'with invalid versions' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:version) do
+        [
+          '1',
+          '1.2',
+          '1./2.3',
+          '../../../../../1.2.3',
+          '%2e%2e%2f1.2.3'
+        ]
+      end
+
+      with_them do
+        it { expect { subject }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Version is invalid') }
+      end
+    end
   end
 end

ee/spec/services/packages/npm/create_tag_service_spec.rb

@@ -37,7 +37,7 @@ describe Packages::Npm::CreateTagService do
       end
 
       context 'on same package with different version' do
-        let!(:package2) { create(:npm_package, project: package.project, name: package.name, version: '5.0.0testing') }
+        let!(:package2) { create(:npm_package, project: package.project, name: package.name, version: '5.0.0-testing') }
 
         it { expect { subject }.to not_change { Packages::Tag.count } }
         it { expect(subject.name).to eq(tag_name) }