diff options
-rw-r--r-- | app/services/import/validate_remote_git_endpoint_service.rb | 81 | ||||
-rw-r--r-- | spec/requests/api/projects_spec.rb | 5 | ||||
-rw-r--r-- | spec/services/import/validate_remote_git_endpoint_service_spec.rb | 43 |
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 |