Add latest changes from gitlab-org/security/gitlab@14-6-stable-ee

2 files changed, 61 insertions, 9 deletions

diff --git a/app/services/packages/npm/create_package_service.rb b/app/services/packages/npm/create_package_service.rb
index 655616c3a28..76a7f3bdc72 100644
--- a/app/services/packages/npm/create_package_service.rb
+++ b/app/services/packages/npm/create_package_service.rb
@@ -70,10 +70,24 @@ module Packages
         end
       end
 
+      # TODO (technical debt): Extract the package size calculation to its own component and unit test it separately.
+      def calculated_package_file_size
+        strong_memoize(:calculated_package_file_size) do
+          # This calculation is based on:
+          # 1. 4 chars in a Base64 encoded string are 3 bytes in the original string. Meaning 1 char is 0.75 bytes.
+          # 2. The encoded string may have 1 or 2 extra '=' chars used for padding. Each padding char means 1 byte less in the original string.
+          # Reference:
+          # - https://blog.aaronlenoir.com/2017/11/10/get-original-length-from-base-64-string/
+          # - https://en.wikipedia.org/wiki/Base64#Decoding_Base64_with_padding
+          encoded_data = attachment['data']
+          ((encoded_data.length * 0.75 ) - encoded_data[-2..].count('=')).to_i
+        end
+      end
+
       def file_params
         {
           file:      CarrierWaveStringFile.new(Base64.decode64(attachment['data'])),
-          size:      attachment['length'],
+          size:      calculated_package_file_size,
           file_sha1: version_data[:dist][:shasum],
           file_name: package_file_name,
           build:     params[:build]
@@ -86,7 +100,7 @@ module Packages
       end
 
       def file_size_exceeded?
-        project.actual_limits.exceeded?(:npm_max_file_size, attachment['length'].to_i)
+        project.actual_limits.exceeded?(:npm_max_file_size, calculated_package_file_size)
       end
     end
   end
diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb
index 3bb675058df..a5b36527565 100644
--- a/spec/services/packages/npm/create_package_service_spec.rb
+++ b/spec/services/packages/npm/create_package_service_spec.rb
@@ -11,10 +11,8 @@ RSpec.describe Packages::Npm::CreatePackageService do
     Gitlab::Json.parse(fixture_file('packages/npm/payload.json')
         .gsub('@root/npm-test', package_name)
         .gsub('1.0.1', version)).with_indifferent_access
-      .merge!(override)
   end
 
-  let(:override) { {} }
   let(:package_name) { "@#{namespace.path}/my-app" }
   let(:version_data) { params.dig('versions', '1.0.1') }
 
@@ -116,13 +114,53 @@ RSpec.describe Packages::Npm::CreatePackageService do
       it { expect(subject[:message]).to be 'Package already exists.' }
     end
 
-    context 'file size above maximum limit' do
+    describe 'max file size validation' do
+      let(:max_file_size) { 5.bytes}
+
+      shared_examples_for 'max file size validation failure' do
+        it 'returns a 400 error', :aggregate_failures do
+          expect(subject[:http_status]).to eq 400
+          expect(subject[:message]).to be 'File is too large.'
+        end
+      end
+
       before do
-        params['_attachments']["#{package_name}-#{version}.tgz"]['length'] = project.actual_limits.npm_max_file_size + 1
+        project.actual_limits.update!(npm_max_file_size: max_file_size)
       end
 
-      it { expect(subject[:http_status]).to eq 400 }
-      it { expect(subject[:message]).to be 'File is too large.' }
+      context 'when max file size is exceeded' do
+        # NOTE: The base64 encoded package data in the fixture file is the "hello\n" string, whose byte size is 6.
+        it_behaves_like 'max file size validation failure'
+      end
+
+      context 'when file size is faked by setting the attachment length param to a lower size' do
+        let(:params) { super().deep_merge!( { _attachments: { "#{package_name}-#{version}.tgz" => { data: encoded_package_data, length: 1 } } }) }
+
+        # TODO (technical debt): Extract the package size calculation outside the service and add separate specs for it.
+        # Right now we have several contexts here to test the calculation's different scenarios.
+        context "when encoded package data is not padded" do
+          # 'Hello!' (size = 6 bytes) => 'SGVsbG8h'
+          let(:encoded_package_data) { 'SGVsbG8h' }
+
+          it_behaves_like 'max file size validation failure'
+        end
+
+        context "when encoded package data is padded with '='" do
+          let(:max_file_size) { 4.bytes}
+          # 'Hello' (size = 5 bytes) => 'SGVsbG8='
+          let(:encoded_package_data) { 'SGVsbG8=' }
+
+          it_behaves_like 'max file size validation failure'
+        end
+
+        context "when encoded package data is padded with '=='" do
+          let(:max_file_size) { 3.bytes}
+          # 'Hell' (size = 4 bytes) => 'SGVsbA=='
+          let(:encoded_package_data) { 'SGVsbA==' }
+
+          it_behaves_like 'max file size validation failure'
+        end
+      end
     end
 
     [
@@ -141,7 +179,7 @@ RSpec.describe Packages::Npm::CreatePackageService do
     end
 
     context 'with empty versions' do
-      let(:override) { { versions: {} } }
+      let(:params) { super().merge!({ versions: {} } ) }
 
       it { expect(subject[:http_status]).to eq 400 }
       it { expect(subject[:message]).to eq 'Version is empty.' }