Improve validation for proxy in http_client_buildernls/proxy-bug

Ensures that proxy_uri will only return nil or a URI object, but never
an empty string.

2 files changed, 52 insertions, 8 deletions

diff --git a/lib/chef/http/basic_client.rb b/lib/chef/http/basic_client.rb
index de5e7c03a8..4cc3732172 100644
--- a/lib/chef/http/basic_client.rb
+++ b/lib/chef/http/basic_client.rb
@@ -103,14 +103,16 @@ class Chef
         # Check if the proxy string contains a scheme. If not, add the url's scheme to the
         # proxy before parsing. The regex /^.*:\/\// matches, for example, http://. Reusing proxy
         # here since we are really just trying to get the string built correctly.
-        if String === proxy && !proxy.strip.empty?
+        #
+        # If `proxy` is not a string or is an empty string, the value will be nil.
+        proxy = if proxy.is_a?(String) && !proxy.strip.empty?
           if proxy.match(/^.*:\/\//)
-           proxy = URI.parse(proxy.strip)
+            proxy = URI.parse(proxy.strip)
           else
-           proxy = URI.parse("#{url.scheme}://#{proxy.strip}")
-          end 
+            proxy = URI.parse("#{url.scheme}://#{proxy.strip}")
+          end
         end
-        
+
         no_proxy = Chef::Config[:no_proxy] || env['NO_PROXY'] || env['no_proxy']
         excludes = no_proxy.to_s.split(/\s*,\s*/).compact
         excludes = excludes.map { |exclude| exclude =~ /:\d+$/ ? exclude : "#{exclude}:*" }
diff --git a/spec/unit/http/basic_client_spec.rb b/spec/unit/http/basic_client_spec.rb
index b7552f54aa..d68d1d02b9 100644
--- a/spec/unit/http/basic_client_spec.rb
+++ b/spec/unit/http/basic_client_spec.rb
@@ -39,19 +39,57 @@ describe "HTTP Connection" do
     end
   end
 
+  describe "#http_client_builder" do
+    subject(:http_client_builder) { basic_client.http_client_builder }
+
+    context "when the http_proxy is an URI" do
+      before :each do
+        allow(basic_client).to receive(:proxy_uri).and_return(URI.parse(
+          "http://user:pass@example.com:1234"
+        ))
+      end
+
+      it "has a proxy_address" do
+        expect(http_client_builder.proxy_address).to eq "example.com"
+      end
+
+      it "has a proxy_pass" do
+        expect(http_client_builder.proxy_pass).to eq "pass"
+      end
+
+      it "has a proxy_port" do
+        expect(http_client_builder.proxy_port).to eq 1234
+      end
+
+      it "has a proxy_user" do
+        expect(http_client_builder.proxy_user).to eq "user"
+      end
+    end
+
+    context "when the http_proxy is nil" do
+      before :each do
+        allow(basic_client).to receive(:proxy_uri).and_return(nil)
+      end
+
+      it "returns Net::HTTP" do
+        expect(basic_client.http_client_builder).to eq Net::HTTP
+      end
+    end
+  end
+
   describe "#proxy_uri" do
+    subject(:proxy_uri) { basic_client.proxy_uri }
+
     shared_examples_for "a proxy uri" do
       let(:proxy_host) { "proxy.mycorp.com" }
       let(:proxy_port) { 8080 }
       let(:proxy) { "#{proxy_prefix}#{proxy_host}:#{proxy_port}" }
 
       it "should contain the host" do
-        proxy_uri = subject.proxy_uri
         expect(proxy_uri.host).to eq(proxy_host)
       end
 
       it "should contain the port" do
-        proxy_uri = subject.proxy_uri
         expect(proxy_uri.port).to eq(proxy_port)
       end
     end
@@ -122,7 +160,11 @@ describe "HTTP Connection" do
       end
 
       it "to not fail with URI parse exception" do
-        expect { subject.proxy_uri }.to_not raise_error
+        expect { proxy_uri }.to_not raise_error
+      end
+
+      it "returns nil" do
+        expect(proxy_uri).to eq nil
       end
     end
   end