diff options
-rw-r--r-- | app/services/packages/npm/create_package_service.rb | 18 | ||||
-rw-r--r-- | spec/services/packages/npm/create_package_service_spec.rb | 52 |
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.' } |