Leverage existing URI::Generic and Net::HTTP code for proxy handling

Details:
- Replaced custom use_proxy? method with URI::Generic.use_proxy? to use already existing functionality for handling the `no_proxy` string, which can include a list of comma- or space-separated elements.
- Take advantage of the `p_no_proxy` variable in Net::HTTP to maintain environment variables related to proxy settings.
- Note: With this change, wildcards are no longer usable to bypass the proxy due to limitations in the existing proxy code.

Related to #5004

Author: rgisiger

rb/lib/selenium/webdriver/common/proxy.rb

@@ -17,6 +17,8 @@
 # specific language governing permissions and limitations
 # under the License.
 
+# For more information on the proxy settings, see: https://www.w3.org/TR/webdriver2/#dfn-proxy-configuration
+
 module Selenium
   module WebDriver
     class Proxy
@@ -89,7 +91,11 @@ def http=(value)
 
       def no_proxy=(value)
         self.type = :manual
-        @no_proxy = value
+        @no_proxy = if value.is_a?(String)
+                      value.scan(/([^:,\s]+)(:\d+)?/).map(&:join) # adapted from URI::Generic.use_proxy?
+                    else
+                      value
+                    end
       end
 
       def ssl=(value)
@@ -145,7 +151,7 @@ def as_json(*)
           'proxyType' => TYPES[type].downcase,
           'ftpProxy' => ftp,
           'httpProxy' => http,
-          'noProxy' => no_proxy.is_a?(String) ? no_proxy.split(', ') : no_proxy,
+          'noProxy' => no_proxy,
           'proxyAutoconfigUrl' => pac,
           'sslProxy' => ssl,
           'autodetect' => auto_detect,

rb/lib/selenium/webdriver/firefox/profile.rb

@@ -138,7 +138,8 @@ def proxy=(proxy)
             set_manual_proxy_preference 'ssl', proxy.ssl
             set_manual_proxy_preference 'socks', proxy.socks
 
-            self['network.proxy.no_proxies_on'] = proxy.no_proxy || ''
+            # cf. http://kb.mozillazine.org/Network.proxy.no_proxies_on
+            self['network.proxy.no_proxies_on'] = proxy.no_proxy&.join(',') || ''
           when :pac
             self['network.proxy.type'] = 2
             self['network.proxy.autoconfig_url'] = proxy.pac

rb/lib/selenium/webdriver/remote/http/default.rb

@@ -16,7 +16,7 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-require 'ipaddr'
+require 'uri/generic'
 
 module Selenium
   module WebDriver
@@ -88,7 +88,7 @@ def request(verb, url, headers, payload, redirects = 0)
               sleep 2
               retry
             rescue Errno::ECONNREFUSED => e
-              raise e.class, "using proxy: #{proxy.http}" if use_proxy?
+              raise e.class, "using proxy: #{http.proxy_uri}" if http.proxy?
 
               raise
             end
@@ -119,18 +119,20 @@ def response_for(request)
           end
 
           def new_http_client
-            if use_proxy?
-              url = @proxy.http
+            if proxy
+              url = proxy.http
               unless proxy.respond_to?(:http) && url
                 raise Error::WebDriverError,
-                      "expected HTTP proxy, got #{@proxy.inspect}"
+                      "expected HTTP proxy, got #{proxy.inspect}"
               end
 
-              proxy = URI.parse(url)
+              proxy_url = URI.parse(url)
 
-              Net::HTTP.new(server_url.host, server_url.port, proxy.host, proxy.port, proxy.user, proxy.password)
+              Net::HTTP.new(server_url.host, server_url.port,
+                            proxy_url.host, proxy_url.port, proxy_url.user, proxy_url.password,
+                            proxy.no_proxy&.join(','))
             else
-              Net::HTTP.new server_url.host, server_url.port
+              Net::HTTP.new(server_url.host, server_url.port, nil)
             end
           end
 
@@ -145,27 +147,6 @@ def proxy
               end
             end
           end
-
-          def use_proxy?
-            return false if proxy.nil?
-
-            if proxy.no_proxy
-              ignored = proxy.no_proxy.split(',').any? do |host|
-                host == '*' ||
-                  host == server_url.host || (
-                begin
-                  IPAddr.new(host).include?(server_url.host)
-                rescue ArgumentError
-                  false
-                end
-              )
-              end
-
-              !ignored
-            else
-              true
-            end
-          end
         end # Default
       end # Http
     end # Remote

rb/sig/lib/selenium/webdriver/remote/http/default.rbs

@@ -39,8 +39,6 @@ module Selenium
           def new_http_client: () -> untyped
 
           def proxy: () -> untyped
-
-          def use_proxy?: () -> untyped
         end
       end
     end

rb/spec/unit/selenium/webdriver/proxy_spec.rb

@@ -26,7 +26,7 @@ module WebDriver
         {
           ftp: 'mythicalftpproxy:21',
           http: 'mythicalproxy:80',
-          no_proxy: 'noproxy',
+          no_proxy: 'noproxy,noproxy2,noproxy3:443,127.0.0.1,127.0.0.1:80',
           ssl: 'mythicalsslproxy',
           socks: 'mythicalsocksproxy:65555',
           socks_username: 'test',
@@ -59,21 +59,27 @@ module WebDriver
 
         expect(proxy.ftp).to            eq(proxy_settings[:ftp])
         expect(proxy.http).to           eq(proxy_settings[:http])
-        expect(proxy.no_proxy).to       eq(proxy_settings[:no_proxy])
+        expect(proxy.no_proxy).to       eq(%w[noproxy noproxy2 noproxy3:443 127.0.0.1 127.0.0.1:80])
         expect(proxy.ssl).to            eq(proxy_settings[:ssl])
         expect(proxy.socks).to          eq(proxy_settings[:socks])
         expect(proxy.socks_username).to eq(proxy_settings[:socks_username])
         expect(proxy.socks_password).to eq(proxy_settings[:socks_password])
         expect(proxy.socks_version).to  eq(proxy_settings[:socks_version])
       end
 
+      it 'allows different kinds of separator for no_proxy string list', :aggregate_failures do
+        proxy = described_class.new({no_proxy: 'noproxy,noproxy2, noproxy3:443 127.0.0.1 , 127.0.0.1:80'})
+
+        expect(proxy.no_proxy).to eq(%w[noproxy noproxy2 noproxy3:443 127.0.0.1 127.0.0.1:80])
+      end
+
       it 'returns a hash of the json properties to serialize', :aggregate_failures do
         proxy_json = described_class.new(proxy_settings).as_json
 
         expect(proxy_json['proxyType']).to     eq('manual')
         expect(proxy_json['ftpProxy']).to      eq(proxy_settings[:ftp])
         expect(proxy_json['httpProxy']).to     eq(proxy_settings[:http])
-        expect(proxy_json['noProxy']).to       eq([proxy_settings[:no_proxy]])
+        expect(proxy_json['noProxy']).to       eq(%w[noproxy noproxy2 noproxy3:443 127.0.0.1 127.0.0.1:80])
         expect(proxy_json['sslProxy']).to      eq(proxy_settings[:ssl])
         expect(proxy_json['socksProxy']).to    eq(proxy_settings[:socks])
         expect(proxy_json['socksUsername']).to eq(proxy_settings[:socks_username])
@@ -95,7 +101,7 @@ module WebDriver
         expect(proxy_json['autodetect']).to be true
       end
 
-      it 'onlies add settings that are not nil', :aggregate_failures do
+      it 'only add settings that are not nil', :aggregate_failures do
         settings = {type: :manual, http: 'http proxy'}
 
         proxy = described_class.new(settings)

rb/spec/unit/selenium/webdriver/remote/http/default_spec.rb

@@ -26,7 +26,7 @@ module Http
         describe Default do
           let(:client) do
             client = described_class.new
-            client.server_url = URI.parse('http://example.com')
+            client.server_url = URI.parse('http://test.example.com')
 
             client
           end
@@ -60,7 +60,7 @@ module Http
             expect(http.proxy_user).to eq('foo')
             expect(http.proxy_pass).to eq('bar')
 
-            expect(http.address).to eq('example.com')
+            expect(http.address).to eq('test.example.com')
           end
 
           it 'raises an error if the proxy is not an HTTP proxy' do
@@ -96,6 +96,11 @@ module Http
                 http = client.send :http
                 expect(http).not_to be_proxy
               end
+
+              with_env('http_proxy' => 'proxy.org:8080', no_proxy_var => 'test.example.com') do
+                http = client.send :http
+                expect(http).not_to be_proxy
+              end
             end
 
             it "ignores the #{no_proxy_var} environment variable when not matching" do
@@ -106,6 +111,14 @@ module Http
                 expect(http.proxy_address).to eq('proxy.org')
                 expect(http.proxy_port).to eq(8080)
               end
+
+              with_env('http_proxy' => 'proxy.org:8080', no_proxy_var => 'ample.com') do
+                http = client.send :http
+
+                expect(http).to be_proxy
+                expect(http.proxy_address).to eq('proxy.org')
+                expect(http.proxy_port).to eq(8080)
+              end
             end
 
             it "understands a comma separated list of domains in #{no_proxy_var}" do
@@ -115,6 +128,13 @@ module Http
               end
             end
 
+            it "understands a space separated list of domains in #{no_proxy_var}" do
+              with_env('http_proxy' => 'proxy.org:8080', no_proxy_var => 'example.com foo.com') do
+                http = client.send :http
+                expect(http).not_to be_proxy
+              end
+            end
+
             it "understands subnetting in #{no_proxy_var}" do
               with_env('http_proxy' => 'proxy.org:8080', no_proxy_var => 'localhost,127.0.0.0/8') do
                 client.server_url = URI.parse('http://127.0.0.1:4444/wd/hub')
@@ -123,6 +143,18 @@ module Http
                 expect(http).not_to be_proxy
               end
             end
+
+            it "uses the specified proxy if the wildcard character is used in #{no_proxy_var}" do
+              with_env('http_proxy' => 'proxy.org:8080', no_proxy_var => '*') do
+                http = client.send :http
+                expect(http).to be_proxy
+              end
+
+              with_env('http_proxy' => 'proxy.org:8080', no_proxy_var => '*.example.com') do
+                http = client.send :http
+                expect(http).to be_proxy
+              end
+            end
           end
 
           it 'raises a sane error if a proxy is refusing connections' do