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

3 files changed, 38 insertions, 91 deletions

diff --git a/app/services/import/validate_remote_git_endpoint_service.rb b/app/services/import/validate_remote_git_endpoint_service.rb
index 2177238fddf..a994072c4aa 100644
--- a/app/services/import/validate_remote_git_endpoint_service.rb
+++ b/app/services/import/validate_remote_git_endpoint_service.rb
@@ -13,8 +13,6 @@ module Import
     GIT_PROTOCOL_PKT_LEN = 4
     GIT_MINIMUM_RESPONSE_LENGTH = GIT_PROTOCOL_PKT_LEN + GIT_EXPECTED_FIRST_PACKET_LINE.length
     EXPECTED_CONTENT_TYPE = "application/x-#{GIT_SERVICE_NAME}-advertisement"
-    INVALID_BODY_MESSAGE = 'Not a git repository: Invalid response body'
-    INVALID_CONTENT_TYPE_MESSAGE = 'Not a git repository: Invalid content-type'
 
     def initialize(params)
       @params = params
@@ -32,35 +30,32 @@ module Import
       uri.fragment = nil
       url = Gitlab::Utils.append_path(uri.to_s, "/info/refs?service=#{GIT_SERVICE_NAME}")
 
-      response, response_body = http_get_and_extract_first_chunks(url)
-
-      validate(uri, response, response_body)
-    rescue *Gitlab::HTTP::HTTP_ERRORS => err
-      error_result("HTTP #{err.class.name.underscore} error: #{err.message}")
-    rescue StandardError => err
-      ServiceResponse.error(
-        message: "Internal #{err.class.name.underscore} error: #{err.message}",
-        reason: 500
-      )
-    end
-
-    private
-
-    def http_get_and_extract_first_chunks(url)
-      # We are interested only in the first chunks of the response
-      # So we're using stream_body: true and breaking when receive enough body
-      response = nil
       response_body = ''
-
-      Gitlab::HTTP.get(url, stream_body: true, follow_redirects: false, basic_auth: auth) do |response_chunk|
-        response = response_chunk
-        response_body += response_chunk
-        break if GIT_MINIMUM_RESPONSE_LENGTH <= response_body.length
+      result = nil
+      Gitlab::HTTP.try_get(url, stream_body: true, follow_redirects: false, basic_auth: auth) do |fragment|
+        response_body += fragment
+        next if response_body.length < GIT_MINIMUM_RESPONSE_LENGTH
+
+        result = if status_code_is_valid(fragment) && content_type_is_valid(fragment) && response_body_is_valid(response_body)
+                   :success
+                 else
+                   :error
+                 end
+
+        # We are interested only in the first chunks of the response
+        # So we're using stream_body: true and breaking when receive enough body
+        break
       end
 
-      [response, response_body]
+      if result == :success
+        ServiceResponse.success
+      else
+        ServiceResponse.error(message: "#{uri} is not a valid HTTP Git repository")
+      end
     end
 
+    private
+
     def auth
       unless @params[:user].to_s.blank?
         {
@@ -70,38 +65,16 @@ module Import
       end
     end
 
-    def validate(uri, response, response_body)
-      return status_code_error(uri, response) unless status_code_is_valid?(response)
-      return error_result(INVALID_CONTENT_TYPE_MESSAGE) unless content_type_is_valid?(response)
-      return error_result(INVALID_BODY_MESSAGE) unless response_body_is_valid?(response_body)
-
-      ServiceResponse.success
-    end
-
-    def status_code_error(uri, response)
-      http_code = response.http_response.code.to_i
-      message = response.http_response.message || Rack::Utils::HTTP_STATUS_CODES[http_code]
-
-      error_result(
-        "#{uri} endpoint error: #{http_code}#{message.presence&.prepend(' ')}",
-        http_code
-      )
-    end
-
-    def error_result(message, reason = nil)
-      ServiceResponse.error(message: message, reason: reason)
-    end
-
-    def status_code_is_valid?(response)
-      response.http_response.code == '200'
+    def status_code_is_valid(fragment)
+      fragment.http_response.code == '200'
     end
 
-    def content_type_is_valid?(response)
-      response.http_response['content-type'] == EXPECTED_CONTENT_TYPE
+    def content_type_is_valid(fragment)
+      fragment.http_response['content-type'] == EXPECTED_CONTENT_TYPE
     end
 
-    def response_body_is_valid?(response_body)
-      response_body.length <= GIT_MINIMUM_RESPONSE_LENGTH && response_body.match?(GIT_BODY_MESSAGE_REGEXP)
+    def response_body_is_valid(response_body)
+      response_body.match?(GIT_BODY_MESSAGE_REGEXP)
     end
   end
 end
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index aa7120e482a..64e010aa50f 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -1400,14 +1400,13 @@ RSpec.describe API::Projects, :aggregate_failures, feature_category: :groups_and
     it 'disallows creating a project with an import_url that is not reachable' do
       url = 'http://example.com'
       endpoint_url = "#{url}/info/refs?service=git-upload-pack"
-      error_response = { status: 301, body: '', headers: nil }
-      stub_full_request(endpoint_url, method: :get).to_return(error_response)
+      stub_full_request(endpoint_url, method: :get).to_return({ status: 301, body: '', headers: nil })
       project_params = { import_url: url, path: 'path-project-Foo', name: 'Foo Project' }
 
       expect { post api(path, user), params: project_params }.not_to change { Project.count }
 
       expect(response).to have_gitlab_http_status(:unprocessable_entity)
-      expect(json_response['message']).to eq("#{url} endpoint error: #{error_response[:status]}")
+      expect(json_response['message']).to eq("#{url} is not a valid HTTP Git repository")
     end
 
     it 'creates a project with an import_url that is valid' do
diff --git a/spec/services/import/validate_remote_git_endpoint_service_spec.rb b/spec/services/import/validate_remote_git_endpoint_service_spec.rb
index 15e80f2c85d..1d2b3975832 100644
--- a/spec/services/import/validate_remote_git_endpoint_service_spec.rb
+++ b/spec/services/import/validate_remote_git_endpoint_service_spec.rb
@@ -7,9 +7,7 @@ RSpec.describe Import::ValidateRemoteGitEndpointService, feature_category: :impo
 
   let_it_be(:base_url) { 'http://demo.host/path' }
   let_it_be(:endpoint_url) { "#{base_url}/info/refs?service=git-upload-pack" }
-  let_it_be(:endpoint_error_message) { "#{base_url} endpoint error:" }
-  let_it_be(:body_error_message) { described_class::INVALID_BODY_MESSAGE }
-  let_it_be(:content_type_error_message) { described_class::INVALID_CONTENT_TYPE_MESSAGE }
+  let_it_be(:error_message) { "#{base_url} is not a valid HTTP Git repository" }
 
   describe '#execute' do
     let(:valid_response) do
@@ -72,14 +70,13 @@ RSpec.describe Import::ValidateRemoteGitEndpointService, feature_category: :impo
       end
 
       it 'reports error when status code is not 200' do
-        error_response = { status: 401 }
-        stub_full_request(endpoint_url, method: :get).to_return(error_response)
+        stub_full_request(endpoint_url, method: :get).to_return(valid_response.merge({ status: 301 }))
 
         result = subject.execute
 
         expect(result).to be_a(ServiceResponse)
         expect(result.error?).to be(true)
-        expect(result.message).to eq("#{endpoint_error_message} #{error_response[:status]}")
+        expect(result.message).to eq(error_message)
       end
 
       it 'reports error when invalid URL is provided' do
@@ -97,49 +94,27 @@ RSpec.describe Import::ValidateRemoteGitEndpointService, feature_category: :impo
 
         expect(result).to be_a(ServiceResponse)
         expect(result.error?).to be(true)
-        expect(result.message).to eq(content_type_error_message)
-      end
-
-      it 'reports error when body is too short' do
-        stub_full_request(endpoint_url, method: :get).to_return(valid_response.merge({ body: 'invalid content' }))
-
-        result = subject.execute
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq(body_error_message)
+        expect(result.message).to eq(error_message)
       end
 
       it 'reports error when body is in invalid format' do
-        stub_full_request(endpoint_url, method: :get).to_return(valid_response.merge({ body: 'invalid long content with no git respons whatshowever' }))
-
-        result = subject.execute
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq(body_error_message)
-      end
-
-      it 'reports error when http exceptions are raised' do
-        err = SocketError.new('dummy message')
-        stub_full_request(endpoint_url, method: :get).to_raise(err)
+        stub_full_request(endpoint_url, method: :get).to_return(valid_response.merge({ body: 'invalid content' }))
 
         result = subject.execute
 
         expect(result).to be_a(ServiceResponse)
         expect(result.error?).to be(true)
-        expect(result.message).to eq("HTTP #{err.class.name.underscore} error: #{err.message}")
+        expect(result.message).to eq(error_message)
       end
 
-      it 'reports error when other exceptions are raised' do
-        err = StandardError.new('internal dummy message')
-        stub_full_request(endpoint_url, method: :get).to_raise(err)
+      it 'reports error when exception is raised' do
+        stub_full_request(endpoint_url, method: :get).to_raise(SocketError.new('dummy message'))
 
         result = subject.execute
 
         expect(result).to be_a(ServiceResponse)
         expect(result.error?).to be(true)
-        expect(result.message).to eq("Internal #{err.class.name.underscore} error: #{err.message}")
+        expect(result.message).to eq(error_message)
       end
     end