add error handling, set *_proxy instead of *_PROXY

6 files changed, 143 insertions, 76 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a91f9fb2f7..c81d935590 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -44,7 +44,7 @@
 * Add config options for attribute whitelisting in node.save. (CHEF-3811)
 * Use user's .chef as a fallback cache path if /var/chef is not accessible. (CHEF-5259)
 * Fixed Ruby 2.0 Windows compatibility issues around ruby-wmi gem by replacing it with wmi-lite gem.
-* Set PROXY environment variables if preset in config. (CHEF-4712)
+* Set proxy environment variables if preset in config. (CHEF-4712)
 
 ## Release: 11.12.4 (04/30/2014)
 http://www.getchef.com/blog/2014/04/30/release-chef-client-11-12-4-ohai-7-0-4/
diff --git a/DOC_CHANGES.md b/DOC_CHANGES.md
index cf3fdc5bd3..901d79d090 100644
--- a/DOC_CHANGES.md
+++ b/DOC_CHANGES.md
@@ -82,7 +82,7 @@ The default behavior is for the node to save all the attribute data. This can be
 
 We recommend only using `automatic_attribute_whitelist` to reduce the size of the system data being stored for nodes, and discourage the use of the other attribute whitelists except by advanced users.
 
-### Set PROXY environment variables if present in your config file.
+### Set proxy environment variables if present in your config file.
 
 If `:http_proxy`, `:https_proxy`, `:ftp_proxy`, or `:no_proxy` is found in your config file, we will configure your environment variables according to the variable form and configuration info given. If your config file looks like
 
@@ -92,4 +92,4 @@ http_proxy_user "myself"
 http_proxy_pass "Password1"
 ````
 
-then Chef will set `ENV[:HTTP_PROXY] = "http://myself:Password1@proxy.example.org:8080"`
+then Chef will set `ENV['http_proxy'] = "http://myself:Password1@proxy.example.org:8080"`
diff --git a/lib/chef/application.rb b/lib/chef/application.rb
index 24305cfcc4..da06438f8e 100644
--- a/lib/chef/application.rb
+++ b/lib/chef/application.rb
@@ -251,33 +251,36 @@ class Chef::Application
     Chef::Application.fatal!("Aborting due to error in '#{config_file_path}'", 2)
   end
 
-  # Set ENV['HTTP_PROXY']
+  # Set ENV['http_proxy']
   def configure_http_proxy
     if http_proxy = Chef::Config[:http_proxy]
-      env['HTTP_PROXY'] = configure_proxy("http", http_proxy, Chef::Config[:http_proxy_user], Chef::Config[:http_proxy_pass])
+      env['http_proxy'] = configure_proxy("http", http_proxy,
+        Chef::Config[:http_proxy_user], Chef::Config[:http_proxy_pass])
     end
   end
 
-  # Set ENV['HTTPS_PROXY']
+  # Set ENV['https_proxy']
   def configure_https_proxy
     if https_proxy = Chef::Config[:https_proxy]
-      env['HTTPS_PROXY'] = configure_proxy("https", https_proxy, Chef::Config[:https_proxy_user], Chef::Config[:https_proxy_pass])
+      env['https_proxy'] = configure_proxy("https", https_proxy,
+        Chef::Config[:https_proxy_user], Chef::Config[:https_proxy_pass])
     end
   end
 
-  # Set ENV['FTP_PROXY']
+  # Set ENV['ftp_proxy']
   def configure_ftp_proxy
     if ftp_proxy = Chef::Config[:ftp_proxy]
-      env['FTP_PROXY'] = configure_proxy("ftp", ftp_proxy, Chef::Config[:ftp_proxy_user], Chef::Config[:ftp_proxy_pass])
+      env['ftp_proxy'] = configure_proxy("ftp", ftp_proxy,
+        Chef::Config[:ftp_proxy_user], Chef::Config[:ftp_proxy_pass])
     end
   end
 
-  # Set ENV['NO_PROXY']
+  # Set ENV['no_proxy']
   def configure_no_proxy
-    env['NO_PROXY'] = Chef::Config[:no_proxy] if Chef::Config[:no_proxy]
+    env['no_proxy'] = Chef::Config[:no_proxy] if Chef::Config[:no_proxy]
   end
 
-  #Builds a proxy uri. Examples:
+  # Builds a proxy uri. Examples:
   #   http://username:password@hostname:port
   #   https://username@hostname:port
   #   ftp://hostname:port
@@ -286,23 +289,27 @@ class Chef::Application
   #   hostport = hostname:port
   #   user = username
   #   pass = password
-  def configure_proxy(scheme, hostport, user, pass)
-    # URI.split returns the following parts:
-    # [scheme, userinfo, host, port, registry, path, opaque, query, fragment]
-    parts = URI.split(URI.encode(hostport))
-    parts[0] = scheme if parts[0].nil?
-    # URI::Generic.build requires an integer for the port, but URI::split gives
-    # returns a string for the port.
-    parts[3] = parts[3].to_i if parts[3]
-    if user
-      userinfo = URI.encode(URI.encode(user), '@:')
-      if pass
-        userinfo << ":#{URI.encode(URI.encode(pass), '@:')}"
+  def configure_proxy(scheme, path, user, pass)
+    begin
+      path = "#{scheme}://#{path}" unless path.start_with?(scheme)
+      # URI.split returns the following parts:
+      # [scheme, userinfo, host, port, registry, path, opaque, query, fragment]
+      parts = URI.split(URI.encode(path))
+      # URI::Generic.build requires an integer for the port, but URI::split gives
+      # returns a string for the port.
+      parts[3] = parts[3].to_i if parts[3]
+      if user
+        userinfo = URI.encode(URI.encode(user), '@:')
+        if pass
+          userinfo << ":#{URI.encode(URI.encode(pass), '@:')}"
+        end
+        parts[1] = userinfo
       end
-      parts[1] = userinfo
-    end
 
-    URI::Generic.build(parts).to_s
+      return URI::Generic.build(parts).to_s
+    rescue URI::Error => e
+      raise Chef::Exceptions::BadProxyURI, e.message
+    end
   end
 
   # This is a hook for testing
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index 782ecc3fd8..438055acd1 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -329,5 +329,7 @@ class Chef
         super "Unable to acquire lock. Waited #{duration} seconds for #{blocking_pid} to release."
       end
     end
+
+    class BadProxyURI < RuntimeError; end
   end
 end
diff --git a/spec/functional/application_spec.rb b/spec/functional/application_spec.rb
index bed293a80d..ae7bc5aa29 100644
--- a/spec/functional/application_spec.rb
+++ b/spec/functional/application_spec.rb
@@ -43,9 +43,9 @@ describe Chef::Application do
 
     it "saves built proxy to ENV which shell_out can use" do
       so = if windows?
-        shell_out("echo $env:HTTP_PROXY")
+        shell_out("echo $env:http_proxy")
       else
-        shell_out("echo $HTTP_PROXY")
+        shell_out("echo $http_proxy")
       end
 
       so.stdout.should == "http://proxy.example.org:8080\n"
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 5054a59b68..3b1eae0b55 100644
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -121,8 +121,8 @@ describe Chef::Application do
         config_location_pathname
         Pathname.stub(:new).with(config_location).and_return(config_location_pathname)
         File.should_receive(:read).
-          with(config_location).
-          and_return(config_content)
+        with(config_location).
+        and_return(config_content)
       end
 
       it "should configure chef::config from a file" do
@@ -248,7 +248,74 @@ describe Chef::Application do
       @app.stub(:configure_no_proxy).and_return(true)
     end
 
-    describe "when configuring ENV['HTTP_PROXY']" do
+    shared_examples_for "setting ENV['http_proxy']" do
+      before do
+        Chef::Config[:http_proxy] = http_proxy
+      end
+
+      it "should set ENV['http_proxy']" do
+        @app.configure_proxy_environment_variables
+        @env['http_proxy'].should == "http://#{address}:#{port}"
+      end
+
+      describe "when Chef::Config[:http_proxy_user] is set" do
+        before do
+          Chef::Config[:http_proxy_user] = "username"
+        end
+
+        it "should set ENV['http_proxy'] with the username" do
+          @app.configure_proxy_environment_variables
+          @env['http_proxy'].should == "http://username@#{address}:#{port}"
+        end
+
+        context "when :http_proxy_user contains '@' and/or ':'" do
+          before do
+            Chef::Config[:http_proxy_user] = "my:usern@me"
+          end
+
+          it "should set ENV['http_proxy'] with the escaped username" do
+            @app.configure_proxy_environment_variables
+            @env['http_proxy'].should == "http://my%3Ausern%40me@#{address}:#{port}"
+          end
+        end
+
+        describe "when Chef::Config[:http_proxy_pass] is set" do
+          before do
+            Chef::Config[:http_proxy_pass] = "password"
+          end
+
+          it "should set ENV['http_proxy'] with the password" do
+            @app.configure_proxy_environment_variables
+            @env['http_proxy'].should == "http://username:password@#{address}:#{port}"
+          end
+
+          context "when :http_proxy_pass contains '@' and/or ':'" do
+            before do
+              Chef::Config[:http_proxy_pass] = ":P@ssword101"
+            end
+
+            it "should set ENV['http_proxy'] with the escaped password" do
+              @app.configure_proxy_environment_variables
+              @env['http_proxy'].should == "http://username:%3AP%40ssword101@#{address}:#{port}"
+            end
+          end
+        end
+      end
+
+      describe "when Chef::Config[:http_proxy_pass] is set (but not Chef::Config[:http_proxy_user])" do
+        before do
+          Chef::Config[:http_proxy_user] = nil
+          Chef::Config[:http_proxy_pass] = "password"
+        end
+
+        it "should set ENV['http_proxy']" do
+          @app.configure_proxy_environment_variables
+          @env['http_proxy'].should == "http://#{address}:#{port}"
+        end
+      end
+    end
+
+    describe "when configuring ENV['http_proxy']" do
       before do
         @env = {}
         @app.stub(:env).and_return(@env)
@@ -263,71 +330,62 @@ describe Chef::Application do
           Chef::Config[:http_proxy] = nil
         end
 
-        it "should not set ENV['HTTP_PROXY']" do
+        it "should not set ENV['http_proxy']" do
           @app.configure_proxy_environment_variables
           @env.should == {}
         end
       end
 
       describe "when Chef::Config[:http_proxy] is set" do
-        before do
-          Chef::Config[:http_proxy] = "http://proxy.example.org:8080"
-        end
-
-        it "should set ENV['HTTP_PROXY'] to http://proxy.example.org:8080" do
-          @app.configure_proxy_environment_variables
-          @env['HTTP_PROXY'].should == "http://proxy.example.org:8080"
-        end
+        context "when given an FQDN" do
+          let(:address) { "proxy.example.org" }
+          let(:port) { 8080 }
+          let(:http_proxy) { "http://#{address}:#{port}" }
 
-        it "should percent encode the proxy, if necessary" do
-          Chef::Config[:http_proxy] = "http://needs\\some escaping:1234"
-          @app.configure_proxy_environment_variables
-          @env['HTTP_PROXY'].should == "http://needs%5Csome%20escaping:1234"
+          it_should_behave_like "setting ENV['http_proxy']"
         end
 
-        describe "when Chef::Config[:http_proxy_user] is set" do
-          before do
-            Chef::Config[:http_proxy_user] = "username"
-          end
+        context "when given an IP" do
+          let(:address) { "127.0.0.1" }
+          let(:port) { 22 }
+          let(:http_proxy) { "http://#{address}:#{port}" }
 
-          it "should set ENV['HTTP_PROXY'] to http://username@proxy.example.org:8080" do
-            @app.configure_proxy_environment_variables
-            @env['HTTP_PROXY'].should == "http://username@proxy.example.org:8080"
-          end
+          it_should_behave_like "setting ENV['http_proxy']"
+        end
 
-          it "should percent encode the username, including @ and : characters" do
-            Chef::Config[:http_proxy_user] = "K:tty C@t"
-            @app.configure_proxy_environment_variables
-            @env['HTTP_PROXY'].should == "http://K%3Atty%20C%40t@proxy.example.org:8080"
-          end
+        context "when given an IPv6" do
+          let(:address) { "[2001:db8::1]" }
+          let(:port) { 80 }
+          let(:http_proxy) { "http://#{address}:#{port}" }
 
-          describe "when Chef::Config[:http_proxy_pass] is set" do
-            before do
-              Chef::Config[:http_proxy_pass] = "password"
-            end
+          it_should_behave_like "setting ENV['http_proxy']"
+        end
 
-            it "should set ENV['HTTP_PROXY'] to http://username:password@proxy.example.org:8080" do
-              @app.configure_proxy_environment_variables
-              @env['HTTP_PROXY'].should == "http://username:password@proxy.example.org:8080"
-            end
+        context "when given without including http://" do
+          let(:address) { "proxy.example.org" }
+          let(:port) { 8181 }
+          let(:http_proxy) { "#{address}:#{port}" }
 
-            it "should fully percent escape the password, including @ and : characters" do
-              Chef::Config[:http_proxy_pass] = ":P@ssword101"
-              @app.configure_proxy_environment_variables
-              @env['HTTP_PROXY'].should == "http://username:%3AP%40ssword101@proxy.example.org:8080"
-            end
-          end
+          it_should_behave_like "setting ENV['http_proxy']"
         end
 
-        describe "when Chef::Config[:http_proxy_pass] is set (but not Chef::Config[:http_proxy_user])" do
+        context "when given the full proxy in :http_proxy only" do
           before do
+            Chef::Config[:http_proxy] = "http://username:password@proxy.example.org:2222"
             Chef::Config[:http_proxy_user] = nil
-            Chef::Config[:http_proxy_pass] = "password"
+            Chef::Config[:http_proxy_pass] = nil
           end
 
-          it "should set ENV['HTTP_PROXY'] to http://proxy.example.org:8080" do
+          it "should set ENV['http_proxy']" do
             @app.configure_proxy_environment_variables
-            @env['HTTP_PROXY'].should == "http://proxy.example.org:8080"
+            @env['http_proxy'].should == Chef::Config[:http_proxy]
+          end
+        end
+
+        context "when the config options aren't URI compliant" do
+          it "raises Chef::Exceptions::BadProxyURI" do
+            Chef::Config[:http_proxy] = "http://proxy.bad_example.org/:8080"
+            expect { @app.configure_proxy_environment_variables }.to raise_error(Chef::Exceptions::BadProxyURI)
           end
         end
       end