Different approach to monkey-patching regexp

Dockerfile

@@ -1,6 +1,6 @@
 FROM ruby:3.0.0
 
-RUN apt update && apt-get install -y vim
+RUN apt update && apt-get install -y vim tmux tig
 
 WORKDIR app
 COPY Gemfile ssrf_filter.gemspec .

lib/ssrf_filter.rb

@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-require 'ssrf_filter/patch/resolv'
 require 'ssrf_filter/patch/ssl_socket'
 require 'ssrf_filter/ssrf_filter'
 require 'ssrf_filter/version'

lib/ssrf_filter/patch/ssl_socket.rb

@@ -3,36 +3,36 @@
 class SsrfFilter
   module Patch
     module SSLSocket
-      # When fetching a url we'd like to have the following workflow:
-      # 1) resolve the hostname www.example.com, and choose a public ip address to connect to
-      # 2) connect to that specific ip address, to prevent things like DNS TOCTTOU bugs or other trickery
-      #
-      # Ideally this would happen by the ruby http library giving us control over DNS resolution,
-      # but it doesn't. Instead, when making the request we set the uri.hostname to the chosen ip address,
-      # and send a 'Host' header of the original hostname, i.e. connect to 'http://93.184.216.34/' and send
-      # a 'Host: www.example.com' header.
-      #
-      # This works for the http case, http://www.example.com. For the https case, this causes certificate
-      # validation failures, since the server certificate for https://www.example.com will not have a
-      # Subject Alternate Name for 93.184.216.34.
-      #
-      # Thus we perform the monkey-patch below, modifying SSLSocket's `post_connection_check(hostname)`
-      # and `hostname=(hostname)` methods:
-      # If our fiber local variable is set, use that for the hostname instead, otherwise behave as usual.
-      # The only time the variable will be set is if you are executing inside a `with_forced_hostname` block,
-      # which is used in ssrf_filter.rb.
-      #
-      # An alternative approach could be to pass in our own OpenSSL::X509::Store with a custom
-      # `verify_callback` to the ::Net::HTTP.start call, but this would require reimplementing certification
-      # validation, which is dangerous. This way we can piggyback on the existing validation and simply pretend
-      # that we connected to the desired hostname.
-
       def self.apply!
         return if instance_variable_defined?(:@patched_ssl_socket)
 
         @patched_ssl_socket = true
 
         ::OpenSSL::SSL::SSLSocket.class_eval do
+          # When fetching a url we'd like to have the following workflow:
+          # 1) resolve the hostname www.example.com, and choose a public ip address to connect to
+          # 2) connect to that specific ip address, to prevent things like DNS TOCTTOU bugs or other trickery
+          #
+          # Ideally this would happen by the ruby http library giving us control over DNS resolution,
+          # but it doesn't. Instead, when making the request we set the uri.hostname to the chosen ip address,
+          # and send a 'Host' header of the original hostname, i.e. connect to 'http://93.184.216.34/' and send
+          # a 'Host: www.example.com' header.
+          #
+          # This works for the http case, http://www.example.com. For the https case, this causes certificate
+          # validation failures, since the server certificate for https://www.example.com will not have a
+          # Subject Alternate Name for 93.184.216.34.
+          #
+          # Thus we perform the monkey-patch below, modifying SSLSocket's `post_connection_check(hostname)`
+          # and `hostname=(hostname)` methods:
+          # If our fiber local variable is set, use that for the hostname instead, otherwise behave as usual.
+          # The only time the variable will be set is if you are executing inside a `with_forced_hostname` block,
+          # which is used in ssrf_filter.rb.
+          #
+          # An alternative approach could be to pass in our own OpenSSL::X509::Store with a custom
+          # `verify_callback` to the ::Net::HTTP.start call, but this would require reimplementing certification
+          # validation, which is dangerous. This way we can piggyback on the existing validation and simply pretend
+          # that we connected to the desired hostname.
+
           original_post_connection_check = instance_method(:post_connection_check)
           define_method(:post_connection_check) do |hostname|
             original_post_connection_check.bind(self).call(::Thread.current[::SsrfFilter::FIBER_HOSTNAME_KEY] ||
@@ -45,6 +45,20 @@ def self.apply!
               original_hostname.bind(self).call(::Thread.current[::SsrfFilter::FIBER_HOSTNAME_KEY] || hostname)
             end
           end
+
+          # This patch is the successor to https://github.com/arkadiyt/ssrf_filter/pull/54
+          # Due to some changes in Ruby's net/http library (namely https://github.com/ruby/net-http/pull/36),
+          # the SSLSocket in the request was no longer getting `s.hostname` set.
+          # The original fix tried to monkey-patch the Regexp class to cause the original code path to execute,
+          # but this caused other problems (like https://github.com/arkadiyt/ssrf_filter/issues/61)
+          # This fix attempts a different approach to set the hostname on the socket
+          original_initialize = instance_method(:initialize)
+          define_method(:initialize) do |*args|
+            original_initialize.bind(self).call(*args)
+            if ::Thread.current.key?(::SsrfFilter::FIBER_HOSTNAME_KEY)
+              self.hostname = ::Thread.current[::SsrfFilter::FIBER_HOSTNAME_KEY]
+            end
+          end
         end
       end
     end

lib/ssrf_filter/ssrf_filter.rb

@@ -84,7 +84,6 @@ def self.prefixlen_from_ipaddr(ipaddr)
   }.freeze
 
   FIBER_HOSTNAME_KEY = :__ssrf_filter_hostname
-  FIBER_ADDRESS_KEY = :__ssrf_filter_address
 
   class Error < ::StandardError
   end
@@ -107,7 +106,6 @@ class CRLFInjection < Error
   %i[get put post delete head patch].each do |method|
     define_singleton_method(method) do |url, options = {}, &block|
       ::SsrfFilter::Patch::SSLSocket.apply!
-      ::SsrfFilter::Patch::Resolv.apply!
 
       original_url = url
       scheme_whitelist = options[:scheme_whitelist] || DEFAULT_SCHEME_WHITELIST
@@ -189,7 +187,7 @@ def self.fetch_once(uri, ip, verb, options, &block)
     http_options = options[:http_options] || {}
     http_options[:use_ssl] = (uri.scheme == 'https')
 
-    with_forced_hostname(hostname, ip) do
+    with_forced_hostname(hostname) do
       ::Net::HTTP.start(uri.hostname, uri.port, **http_options) do |http|
         response = http.request(request) do |res|
           block&.call(res)
@@ -221,13 +219,11 @@ def self.validate_request(request)
   end
   private_class_method :validate_request
 
-  def self.with_forced_hostname(hostname, ip, &_block)
+  def self.with_forced_hostname(hostname, &_block)
     ::Thread.current[FIBER_HOSTNAME_KEY] = hostname
-    ::Thread.current[FIBER_ADDRESS_KEY] = ip
     yield
   ensure
     ::Thread.current[FIBER_HOSTNAME_KEY] = nil
-    ::Thread.current[FIBER_ADDRESS_KEY] = nil
   end
   private_class_method :with_forced_hostname
 end

spec/lib/ssrf_filter_spec.rb

@@ -146,27 +146,21 @@
   describe 'with_forced_hostname' do
     it 'sets the value for the block and clear it afterwards' do
       expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil
-      expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil
-      described_class.with_forced_hostname('test', '1.2.3.4') do
+      described_class.with_forced_hostname('test') do
         expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to eq('test')
-        expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to eq('1.2.3.4')
       end
       expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil
-      expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil
     end
 
     it 'clears the value even if an exception is raised' do
       expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil
-      expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil
       expect do
-        described_class.with_forced_hostname('test', '1.2.3.4') do
+        described_class.with_forced_hostname('test') do
           expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to eq('test')
-          expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to eq('1.2.3.4')
           raise StandardError
         end
       end.to raise_error(StandardError)
       expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil
-      expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil
     end
   end