From c44a96733fd967c043cacd75cbaa36ce4bb4f986 Mon Sep 17 00:00:00 2001
From: Furkan Ayhan <furkanayhn@gmail.com>
Date: Mon, 25 Sep 2023 11:04:16 +0000
Subject: [PATCH] Enable gitlab-http gem and its patches
- Adding the gem to the Gemfile
- Initializing it and require its dependencies
- Removing the old patches
---
.rubocop_todo/rspec/file_path.yml | 1 +
Gemfile | 1 +
Gemfile.lock | 11 ++++
config/initializers/gitlab_http.rb | 26 ++++++++++
config/initializers/http_hostname_override.rb | 52 -------------------
config/initializers/net_http_patch.rb | 39 --------------
.../initializers/net_http_response_patch.rb | 48 -----------------
.../lib/net_http/response_patch.rb | 11 ++--
.../gitlab/http_v2/net_http_patch_spec.rb | 2 +-
spec/initializers/gitlab_http_spec.rb | 47 +++++++++++++++++
spec/initializers/net_http_patch_spec.rb | 8 +--
.../net_http_response_patch_spec.rb | 3 ++
12 files changed, 100 insertions(+), 149 deletions(-)
create mode 100644 config/initializers/gitlab_http.rb
delete mode 100644 config/initializers/http_hostname_override.rb
delete mode 100644 config/initializers/net_http_patch.rb
delete mode 100644 config/initializers/net_http_response_patch.rb
create mode 100644 spec/initializers/gitlab_http_spec.rb
diff --git a/.rubocop_todo/rspec/file_path.yml b/.rubocop_todo/rspec/file_path.yml
index ea19394890944..06832184c04e8 100644
--- a/.rubocop_todo/rspec/file_path.yml
+++ b/.rubocop_todo/rspec/file_path.yml
@@ -24,6 +24,7 @@ RSpec/FilePath:
- 'spec/benchmarks/banzai_benchmark.rb'
- 'spec/docs_screenshots/container_registry_docs.rb'
- 'spec/docs_screenshots/wiki_docs.rb'
+ - 'spec/initializers/gitlab_http_spec.rb'
- 'spec/lib/error_tracking/sentry_client/event_spec.rb'
- 'spec/lib/gitlab/import_export/import_export_spec.rb'
- 'spec/lib/gitlab/mail_room/mail_room_spec.rb'
diff --git a/Gemfile b/Gemfile
index ec3e8d3b0009d..b96b331d1e6ac 100644
--- a/Gemfile
+++ b/Gemfile
@@ -349,6 +349,7 @@ gem 'sentry-sidekiq', '~> 5.8.0'
gem 'pg_query', '~> 4.2.3'
gem 'gitlab-schema-validation', path: 'gems/gitlab-schema-validation'
+gem 'gitlab-http', path: 'gems/gitlab-http'
gem 'premailer-rails', '~> 1.10.3'
diff --git a/Gemfile.lock b/Gemfile.lock
index d6911ef9114c5..931c8e41944f5 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -23,6 +23,16 @@ PATH
error_tracking_open_api (1.0.0)
typhoeus (~> 1.0, >= 1.0.1)
+PATH
+ remote: gems/gitlab-http
+ specs:
+ gitlab-http (0.1.0)
+ activesupport (~> 7.0.6)
+ httparty (~> 0.21.0)
+ ipaddress (~> 0.8.3)
+ nokogiri (~> 1.15.4)
+ railties (~> 7.0.6)
+
PATH
remote: gems/gitlab-rspec
specs:
@@ -1812,6 +1822,7 @@ DEPENDENCIES
gitlab-dangerfiles (~> 4.0.0)
gitlab-experiment (~> 0.8.0)
gitlab-fog-azure-rm (~> 1.8.0)
+ gitlab-http!
gitlab-labkit (~> 0.34.0)
gitlab-license (~> 2.3)
gitlab-mail_room (~> 0.0.23)
diff --git a/config/initializers/gitlab_http.rb b/config/initializers/gitlab_http.rb
new file mode 100644
index 0000000000000..8a84313a7fb92
--- /dev/null
+++ b/config/initializers/gitlab_http.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+# When including this gem, we also initialize the patch / override classes in the gem.
+require 'gitlab-http'
+
+Gitlab::HTTP_V2.configure do |config|
+ config.allowed_internal_uris = [
+ URI::HTTP.build(
+ scheme: Gitlab.config.gitlab.protocol,
+ host: Gitlab.config.gitlab.host,
+ port: Gitlab.config.gitlab.port
+ ),
+ URI::Generic.build(
+ scheme: 'ssh',
+ host: Gitlab.config.gitlab_shell.ssh_host,
+ port: Gitlab.config.gitlab_shell.ssh_port
+ )
+ ]
+
+ config.log_exception_proc = ->(exception, extra_info) do
+ Gitlab::ErrorTracking.log_exception(exception, extra_info)
+ end
+ config.silent_mode_log_info_proc = ->(message, http_method) do
+ Gitlab::SilentMode.log_info(message: message, outbound_http_request_method: http_method)
+ end
+end
diff --git a/config/initializers/http_hostname_override.rb b/config/initializers/http_hostname_override.rb
deleted file mode 100644
index 3d840cd32511c..0000000000000
--- a/config/initializers/http_hostname_override.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-# frozen_string_literal: true
-
-# This override allows passing `@hostname_override` to the SNI protocol,
-# which is used to lookup the correct SSL certificate in the
-# request handshake process.
-#
-# Given we've forced the HTTP request to be sent to the resolved
-# IP address in a few scenarios (e.g.: `Gitlab::HTTP` through
-# `Gitlab::UrlBlocker.validate!`), we need to provide the _original_
-# hostname via SNI in order to have a clean connection setup.
-#
-# This is ultimately needed in order to avoid DNS rebinding attacks
-# through HTTP requests.
-#
-class OpenSSL::SSL::SSLContext
- attr_accessor :hostname_override
-end
-
-class OpenSSL::SSL::SSLSocket
- module HostnameOverride
- # rubocop: disable Gitlab/ModuleWithInstanceVariables
- def hostname=(hostname)
- super(@context.hostname_override || hostname)
- end
-
- def post_connection_check(hostname)
- super(@context.hostname_override || hostname)
- end
- # rubocop: enable Gitlab/ModuleWithInstanceVariables
- end
-
- prepend HostnameOverride
-end
-
-class Net::HTTP
- attr_accessor :hostname_override
-
- SSL_IVNAMES << :@hostname_override
- SSL_ATTRIBUTES << :hostname_override
-
- module HostnameOverride
- def addr_port
- return super unless hostname_override
-
- addr = hostname_override
- default_port = use_ssl? ? Net::HTTP.https_default_port : Net::HTTP.http_default_port
- default_port == port ? addr : "#{addr}:#{port}"
- end
- end
-
- prepend HostnameOverride
-end
diff --git a/config/initializers/net_http_patch.rb b/config/initializers/net_http_patch.rb
deleted file mode 100644
index 8231423e1a518..0000000000000
--- a/config/initializers/net_http_patch.rb
+++ /dev/null
@@ -1,39 +0,0 @@
-# frozen_string_literal: true
-
-# Monkey patch Net::HTTP to fix missing URL decoding for username and password in proxy settings
-#
-# See proposed upstream fix https://github.com/ruby/net-http/pull/5
-# See Ruby-lang issue https://bugs.ruby-lang.org/issues/17542
-# See issue on GitLab https://gitlab.com/gitlab-org/gitlab/-/issues/289836
-
-require 'net/http'
-
-# This file can be removed once Ruby 3.0 is no longer supported:
-# https://gitlab.com/gitlab-org/gitlab/-/issues/396223
-return if Gem::Version.new(Net::HTTP::VERSION) >= Gem::Version.new('0.2.0')
-
-module Net
- class HTTP < Protocol
- def proxy_user
- if environment_variable_is_multiuser_safe? && @proxy_from_env
- user = proxy_uri&.user
- CGI.unescape(user) unless user.nil?
- else
- @proxy_user
- end
- end
-
- def proxy_pass
- if environment_variable_is_multiuser_safe? && @proxy_from_env
- pass = proxy_uri&.password
- CGI.unescape(pass) unless pass.nil?
- else
- @proxy_pass
- end
- end
-
- def environment_variable_is_multiuser_safe?
- ENVIRONMENT_VARIABLE_IS_MULTIUSER_SAFE
- end
- end
-end
diff --git a/config/initializers/net_http_response_patch.rb b/config/initializers/net_http_response_patch.rb
deleted file mode 100644
index 4f3eaeec24a5c..0000000000000
--- a/config/initializers/net_http_response_patch.rb
+++ /dev/null
@@ -1,48 +0,0 @@
-# frozen_string_literal: true
-
-module Net
- class HTTPResponse
- # rubocop: disable Cop/LineBreakAfterGuardClauses
- # rubocop: disable Cop/LineBreakAroundConditionalBlock
- # rubocop: disable Layout/EmptyLineAfterGuardClause
- # rubocop: disable Style/AndOr
- # rubocop: disable Style/CharacterLiteral
- # rubocop: disable Style/InfiniteLoop
-
- # Original method:
- # https://github.com/ruby/ruby/blob/v2_7_5/lib/net/http/response.rb#L54-L69
- #
- # Our changes:
- # - Pass along the `start_time` to `Gitlab::BufferedIo`, so we can raise a timeout
- # if reading the headers takes too long.
- # - Limit the regexes to avoid ReDoS attacks.
- def self.each_response_header(sock)
- start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
- key = value = nil
- while true
- line = sock.is_a?(Gitlab::BufferedIo) ? sock.readuntil("\n", true, start_time) : sock.readuntil("\n", true)
- line = line.sub(/\s{0,10}\z/, '')
- break if line.empty?
- if line[0] == ?\s or line[0] == ?\t and value
- # rubocop:disable Gitlab/NoCodeCoverageComment
- # :nocov:
- value << ' ' unless value.empty?
- value << line.strip
- # :nocov:
- # rubocop:enable Gitlab/NoCodeCoverageComment
- else
- yield key, value if key
- key, value = line.strip.split(/\s{0,10}:\s{0,10}/, 2)
- raise Net::HTTPBadResponse, 'wrong header line format' if value.nil?
- end
- end
- yield key, value if key
- end
- # rubocop: enable Cop/LineBreakAfterGuardClauses
- # rubocop: enable Cop/LineBreakAroundConditionalBlock
- # rubocop: enable Layout/EmptyLineAfterGuardClause
- # rubocop: enable Style/AndOr
- # rubocop: enable Style/CharacterLiteral
- # rubocop: enable Style/InfiniteLoop
- end
-end
diff --git a/gems/gitlab-http/lib/net_http/response_patch.rb b/gems/gitlab-http/lib/net_http/response_patch.rb
index e5477a3131829..303d629b32e4f 100644
--- a/gems/gitlab-http/lib/net_http/response_patch.rb
+++ b/gems/gitlab-http/lib/net_http/response_patch.rb
@@ -20,11 +20,12 @@ def self.each_response_header(sock)
start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
key = value = nil
while true
- line = if sock.is_a?(Gitlab::HTTP_V2::BufferedIo)
- sock.readuntil("\n", true, start_time)
- else
- sock.readuntil("\n", true)
- end
+ uses_buffered_io = sock.is_a?(Gitlab::HTTP_V2::BufferedIo)
+
+ # TODO: Gitlab::BufferedIo is temporarily used for an easy migration.
+ uses_buffered_io ||= sock.is_a?(Gitlab::BufferedIo) if defined?(Gitlab::BufferedIo)
+
+ line = uses_buffered_io ? sock.readuntil("\n", true, start_time) : sock.readuntil("\n", true)
line = line.sub(/\s{0,10}\z/, '')
break if line.empty?
if line[0] == ?\s or line[0] == ?\t and value
diff --git a/gems/gitlab-http/spec/gitlab/http_v2/net_http_patch_spec.rb b/gems/gitlab-http/spec/gitlab/http_v2/net_http_patch_spec.rb
index b82646fb3657e..f34b0d9840359 100644
--- a/gems/gitlab-http/spec/gitlab/http_v2/net_http_patch_spec.rb
+++ b/gems/gitlab-http/spec/gitlab/http_v2/net_http_patch_spec.rb
@@ -3,7 +3,7 @@
require 'spec_helper'
require 'net/http'
-RSpec.describe 'Net::HTTP patch proxy user and password encoding' do
+RSpec.describe 'Net::HTTP patch proxy user and password encoding', feature_category: :shared do
let(:net_http) { Net::HTTP.new('hostname.example') }
before do
diff --git a/spec/initializers/gitlab_http_spec.rb b/spec/initializers/gitlab_http_spec.rb
new file mode 100644
index 0000000000000..7715112abf4aa
--- /dev/null
+++ b/spec/initializers/gitlab_http_spec.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::HTTP_V2, feature_category: :shared do
+ it 'handles log_exception_proc' do
+ expect(Gitlab::HTTP_V2::Client).to receive(:httparty_perform_request)
+ .and_raise(Net::ReadTimeout)
+
+ expect(Gitlab::ErrorTracking).to receive(:log_exception)
+ .with(Net::ReadTimeout, {})
+
+ expect { described_class.get('http://example.org') }.to raise_error(Net::ReadTimeout)
+ end
+
+ context 'when silent_mode_enabled is true' do
+ before do
+ stub_application_setting(silent_mode_enabled: true)
+ end
+
+ context 'when sending a POST request' do
+ it 'handles silent_mode_log_info_proc' do
+ expect(::Gitlab::AppJsonLogger).to receive(:info).with(
+ message: "Outbound HTTP request blocked",
+ outbound_http_request_method: 'Net::HTTP::Post',
+ silent_mode_enabled: true
+ )
+
+ expect { described_class.post('http://example.org', silent_mode_enabled: true) }.to raise_error(
+ Gitlab::HTTP_V2::SilentModeBlockedError
+ )
+ end
+ end
+
+ context 'when sending a GET request' do
+ before do
+ stub_request(:get, 'http://example.org').to_return(body: 'hello')
+ end
+
+ it 'does not raise an error' do
+ expect(::Gitlab::AppJsonLogger).not_to receive(:info)
+
+ expect(described_class.get('http://example.org', silent_mode_enabled: true).body).to eq('hello')
+ end
+ end
+ end
+end
diff --git a/spec/initializers/net_http_patch_spec.rb b/spec/initializers/net_http_patch_spec.rb
index b9f5299b58cfa..959eae954c487 100644
--- a/spec/initializers/net_http_patch_spec.rb
+++ b/spec/initializers/net_http_patch_spec.rb
@@ -1,11 +1,11 @@
# frozen_string_literal: true
-require 'fast_spec_helper'
-require 'net/http'
+# TODO: This spec file can be removed after fully migration to the gitlab-http gem.
+# It's already covered in gems/gitlab-http/spec/gitlab/http_v2/net_http_patch_spec.rb
-require_relative '../../config/initializers/net_http_patch'
+require 'spec_helper'
-RSpec.describe 'Net::HTTP patch proxy user and password encoding' do
+RSpec.describe 'Net::HTTP patch proxy user and password encoding', feature_category: :shared do
let(:net_http) { Net::HTTP.new('hostname.example') }
before do
diff --git a/spec/initializers/net_http_response_patch_spec.rb b/spec/initializers/net_http_response_patch_spec.rb
index cd261d7b997c6..8074047d6aa36 100644
--- a/spec/initializers/net_http_response_patch_spec.rb
+++ b/spec/initializers/net_http_response_patch_spec.rb
@@ -1,5 +1,8 @@
# frozen_string_literal: true
+# TODO: This spec file can be removed after fully migration to the gitlab-http gem.
+# It's already covered in gems/gitlab-http/spec/gitlab/http_v2/net_http_response_patch_spec.rb
+
require 'spec_helper'
RSpec.describe 'Net::HTTPResponse patch header read timeout', feature_category: :shared do
--
GitLab