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

4 files changed, 101 insertions, 29 deletions

diff --git a/app/services/packages/npm/create_package_service.rb b/app/services/packages/npm/create_package_service.rb
index ae9c92a3d3a..f1f589c3188 100644
--- a/app/services/packages/npm/create_package_service.rb
+++ b/app/services/packages/npm/create_package_service.rb
@@ -72,10 +72,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]
@@ -88,7 +102,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/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index 2c5d76ba41d..f092e03046a 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -252,13 +252,13 @@ module Gitlab
       def internal_web?(uri)
         uri.scheme == config.gitlab.protocol &&
           uri.hostname == config.gitlab.host &&
-          (uri.port.blank? || uri.port == config.gitlab.port)
+          get_port(uri) == config.gitlab.port
       end
 
       def internal_shell?(uri)
         uri.scheme == 'ssh' &&
           uri.hostname == config.gitlab_shell.ssh_host &&
-          (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
+          get_port(uri) == config.gitlab_shell.ssh_port
       end
 
       def domain_allowed?(uri)
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index e076815c4f6..0713475d59b 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -531,24 +531,6 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
           end
         end
       end
-
-      def stub_domain_resolv(domain, ip, port = 80, &block)
-        address = instance_double(Addrinfo,
-          ip_address: ip,
-          ipv4_private?: true,
-          ipv6_linklocal?: false,
-          ipv4_loopback?: false,
-          ipv6_loopback?: false,
-          ipv4?: false,
-          ip_port: port
-        )
-        allow(Addrinfo).to receive(:getaddrinfo).with(domain, port, any_args).and_return([address])
-        allow(address).to receive(:ipv6_v4mapped?).and_return(false)
-
-        yield
-
-        allow(Addrinfo).to receive(:getaddrinfo).and_call_original
-      end
     end
 
     context 'when enforce_user is' do
@@ -611,6 +593,44 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
 
       expect(described_class).to be_blocked_url('http://foobar.x')
     end
+
+    context 'when gitlab is running on a non-default port' do
+      let(:gitlab_port) { 3000 }
+
+      before do
+        stub_config(gitlab: { protocol: 'http', host: 'gitlab.local', port: gitlab_port })
+      end
+
+      it 'returns true for url targeting the wrong port' do
+        stub_domain_resolv('gitlab.local', '127.0.0.1') do
+          expect(described_class).to be_blocked_url("http://gitlab.local/foo")
+        end
+      end
+
+      it 'does not block url on gitlab port' do
+        stub_domain_resolv('gitlab.local', '127.0.0.1') do
+          expect(described_class).not_to be_blocked_url("http://gitlab.local:#{gitlab_port}/foo")
+        end
+      end
+    end
+
+    def stub_domain_resolv(domain, ip, port = 80, &block)
+      address = instance_double(Addrinfo,
+        ip_address: ip,
+        ipv4_private?: true,
+        ipv6_linklocal?: false,
+        ipv4_loopback?: false,
+        ipv6_loopback?: false,
+        ipv4?: false,
+        ip_port: port
+      )
+      allow(Addrinfo).to receive(:getaddrinfo).with(domain, port, any_args).and_return([address])
+      allow(address).to receive(:ipv6_v4mapped?).and_return(false)
+
+      yield
+
+      allow(Addrinfo).to receive(:getaddrinfo).and_call_original
+    end
   end
 
   describe '#validate_hostname' do
diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb
index b1beb2adb3b..d007cda44d9 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') }
 
@@ -127,13 +125,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
 
     [
@@ -152,7 +190,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.' }