Add validation to pypi file sha256 values

Merge branch 'security-262724-pypi-sha256-validation-14-10' into '14-10-stable-ee'

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

Changelog: security

app/models/packages/package_file.rb

@@ -35,6 +35,7 @@ class Packages::PackageFile < ApplicationRecord
   validates :file_name, presence: true
 
   validates :file_name, uniqueness: { scope: :package }, if: -> { !pending_destruction? && package&.pypi? }
+  validates :file_sha256, format: { with: Gitlab::Regex.sha256_regex }, if: -> { package&.pypi? }, allow_nil: true
 
   scope :recent, -> { order(id: :desc) }
   scope :limit_recent, ->(limit) { recent.limit(limit) }

lib/api/pypi_packages.rb

@@ -174,7 +174,7 @@ module API
           requires :version, type: String
           optional :requires_python, type: String
           optional :md5_digest, type: String
-          optional :sha256_digest, type: String
+          optional :sha256_digest, type: String, regexp: Gitlab::Regex.sha256_regex
         end
 
         route_setting :authentication, deploy_token_allowed: true, basic_auth_personal_access_token: true, job_token_allowed: :basic_auth

lib/gitlab/regex.rb

@@ -237,6 +237,10 @@ module Gitlab
         generic_package_name_regex
       end
 
+      def sha256_regex
+        @sha256_regex ||= /\A[0-9a-f]{64}\z/i.freeze
+      end
+
       private
 
       def conan_name_regex

spec/lib/gitlab/regex_spec.rb

@@ -1005,4 +1005,19 @@ RSpec.describe Gitlab::Regex do
     it { is_expected.not_to match('.xt.est_') }
     it { is_expected.not_to match('0test1') }
   end
+
+  describe '.sha256_regex' do
+    subject { described_class.sha256_regex }
+
+    it { is_expected.to match('a' * 64) }
+    it { is_expected.to match('abcdefABCDEF1234567890abcdefABCDEF1234567890abcdefABCDEF12345678') }
+    it { is_expected.not_to match('a' * 63) }
+    it { is_expected.not_to match('a' * 65) }
+    it { is_expected.not_to match('a' * 63 + 'g') }
+    it { is_expected.not_to match('a' * 63 + '{') }
+    it { is_expected.not_to match('a' * 63 + '%') }
+    it { is_expected.not_to match('a' * 63 + '*') }
+    it { is_expected.not_to match('a' * 63 + '#') }
+    it { is_expected.not_to match('') }
+  end
 end

spec/models/packages/package_file_spec.rb

@@ -29,19 +29,48 @@ RSpec.describe Packages::PackageFile, type: :model do
 
       let(:package_file) { package.package_files.first }
       let(:status) { :default }
+      let(:file_name) { 'foo' }
       let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
+      let(:params) { { file: file, file_name: file_name, status: status } }
 
-      subject { package.package_files.create!(file: file, file_name: package_file.file_name, status: status) }
+      subject { package.package_files.create!(params) }
 
-      it 'can not save a duplicated file' do
-        expect { subject }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: File name has already been taken")
+      context 'file_name' do
+        let(:file_name) { package_file.file_name }
+
+        it 'can not save a duplicated file' do
+          expect { subject }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: File name has already been taken")
+        end
+
+        context 'with a pending destruction package duplicated file' do
+          let(:status) { :pending_destruction }
+
+          it 'can save it' do
+            expect { subject }.to change { package.package_files.count }.from(1).to(2)
+          end
+        end
       end
 
-      context 'with a pending destruction package duplicated file' do
-        let(:status) { :pending_destruction }
+      context 'file_sha256' do
+        where(:sha256_value, :expected_success) do
+          'a' * 64       | true
+          nil            | true
+          'a' * 63       | false
+          'a' * 65       | false
+          'a' * 63 + '%' | false
+          ''             | false
+        end
+
+        with_them do
+          let(:params) { super().merge({ file_sha256: sha256_value }) }
 
-        it 'can save it' do
-          expect { subject }.to change { package.package_files.count }.from(1).to(2)
+          it 'does not allow invalid sha256 characters' do
+            if expected_success
+              expect { subject }.not_to raise_error
+            else
+              expect { subject }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: File sha256 is invalid")
+            end
+          end
         end
       end
     end

spec/requests/api/pypi_packages_spec.rb

@@ -136,7 +136,7 @@ RSpec.describe API::PypiPackages do
     let(:url) { "/projects/#{project.id}/packages/pypi" }
     let(:headers) { {} }
     let(:requires_python) { '>=3.7' }
-    let(:base_params) { { requires_python: requires_python, version: '1.0.0', name: 'sample-project', sha256_digest: '123' } }
+    let(:base_params) { { requires_python: requires_python, version: '1.0.0', name: 'sample-project', sha256_digest: '1' * 64 } }
     let(:params) { base_params.merge(content: temp_file(file_name)) }
     let(:send_rewritten_field) { true }
     let(:snowplow_gitlab_standard_context) { { project: project, namespace: project.namespace, user: user } }
@@ -221,6 +221,19 @@ RSpec.describe API::PypiPackages do
       it_behaves_like 'returning response status', :bad_request
     end
 
+    context 'with an invalid sha256' do
+      let(:token) { personal_access_token.token }
+      let(:user_headers) { basic_auth_header(user.username, token) }
+      let(:headers) { user_headers.merge(workhorse_headers) }
+
+      before do
+        params[:sha256_digest] = 'a' * 63 + '%'
+        project.add_developer(user)
+      end
+
+      it_behaves_like 'returning response status', :bad_request
+    end
+
     it_behaves_like 'deploy token for package uploads'
 
     it_behaves_like 'job token for package uploads'

spec/services/packages/pypi/create_package_service_spec.rb

@@ -7,6 +7,9 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
 
+  let(:sha256) { '1' * 64 }
+  let(:md5) { '567' }
+
   let(:requires_python) { '>=2.7' }
   let(:params) do
     {
@@ -14,8 +17,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
       version: '1.0',
       content: temp_file('foo.tgz'),
       requires_python: requires_python,
-      sha256_digest: '123',
-      md5_digest: '567'
+      sha256_digest: sha256,
+      md5_digest: md5
     }
   end
 
@@ -34,8 +37,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
         expect(created_package.pypi_metadatum.required_python).to eq '>=2.7'
         expect(created_package.package_files.size).to eq 1
         expect(created_package.package_files.first.file_name).to eq 'foo.tgz'
-        expect(created_package.package_files.first.file_sha256).to eq '123'
-        expect(created_package.package_files.first.file_md5).to eq '567'
+        expect(created_package.package_files.first.file_sha256).to eq sha256
+        expect(created_package.package_files.first.file_md5).to eq md5
       end
     end
 
@@ -74,8 +77,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
       context 'with an existing file' do
         before do
           params[:content] = temp_file('foo.tgz')
-          params[:sha256_digest] = 'abc'
-          params[:md5_digest] = 'def'
+          params[:sha256_digest] = sha256
+          params[:md5_digest] = md5
         end
 
         it 'throws an error' do
@@ -101,8 +104,8 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do
             expect(created_package.pypi_metadatum.required_python).to eq '>=2.7'
             expect(created_package.package_files.size).to eq 1
             expect(created_package.package_files.first.file_name).to eq 'foo.tgz'
-            expect(created_package.package_files.first.file_sha256).to eq 'abc'
-            expect(created_package.package_files.first.file_md5).to eq 'def'
+            expect(created_package.package_files.first.file_sha256).to eq sha256
+            expect(created_package.package_files.first.file_md5).to eq md5
           end
         end
       end