Stop using obsolete URI.escape; use Addressable::URI instead.

See http://stackoverflow.com/questions/2824126/whats-the-difference-between-uri-escape-and-cgi-escape#answer-13059657 for details on why.

7 files changed, 34 insertions, 19 deletions

diff --git a/Gemfile.lock b/Gemfile.lock
index 3ea1b195b6..9df75ab822 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -28,6 +28,7 @@ PATH
   remote: .
   specs:
     chef (12.14.20)
+      addressable
       bundler (>= 1.10)
       chef-config (= 12.14.20)
       chef-zero (~> 4.8)
@@ -56,6 +57,7 @@ PATH
       syslog-logger (~> 1.6)
       uuidtools (~> 2.1.5)
     chef (12.14.20-universal-mingw32)
+      addressable
       bundler (>= 1.10)
       chef-config (= 12.14.20)
       chef-zero (~> 4.8)
diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index fce72e28ff..f46419937a 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -30,6 +30,7 @@ require "chef-config/mixin/fuzzy_hostname_matcher"
 
 require "mixlib/shellout"
 require "uri"
+require "addressable/uri"
 require "openssl"
 
 module ChefConfig
@@ -870,6 +871,13 @@ module ChefConfig
       export_no_proxy(no_proxy) if no_proxy
     end
 
+    # Character classes for Addressable
+    # See https://www.ietf.org/rfc/rfc3986.txt 3.2.1
+    # The user part may not have a : in it
+    USER = Addressable::URI::CharacterClasses::UNRESERVED + Addressable::URI::CharacterClasses::SUB_DELIMS
+    # The password part may have any valid USERINFO characters
+    PASSWORD = USER + "\\:"
+
     # Builds a proxy uri and exports it to the appropriate environment variables. Examples:
     #   http://username:password@hostname:port
     #   https://username@hostname:port
@@ -884,19 +892,17 @@ module ChefConfig
       path = "#{scheme}://#{path}" unless path.include?("://")
       # 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]
+      uri = Addressable::URI.encode(path, Addressable::URI)
+
       if user && !user.empty?
-        userinfo = URI.encode(URI.encode(user), "@:")
+        userinfo = Addressable::URI.encode_component(user, USER)
         if pass
-          userinfo << ":#{URI.encode(URI.encode(pass), '@:')}"
+          userinfo << ":#{Addressable::URI.encode_component(pass, PASSWORD)}"
         end
-        parts[1] = userinfo
+        uri.userinfo = userinfo
       end
 
-      path = URI::Generic.build(parts).to_s
+      path = uri.to_s
       ENV["#{scheme}_proxy".downcase] = path unless ENV["#{scheme}_proxy".downcase]
       ENV["#{scheme}_proxy".upcase] = path unless ENV["#{scheme}_proxy".upcase]
     end
diff --git a/chef.gemspec b/chef.gemspec
index 78db042376..ff30872b9d 100644
--- a/chef.gemspec
+++ b/chef.gemspec
@@ -36,6 +36,7 @@ Gem::Specification.new do |s|
 
   s.add_dependency "plist", "~> 3.2"
   s.add_dependency "iniparse", "~> 1.4"
+  s.add_dependency "addressable"
 
   # Audit mode requires these, so they are non-developmental dependencies now
   %w{rspec-core rspec-expectations rspec-mocks}.each { |gem| s.add_dependency gem, "~> 3.5" }
diff --git a/lib/chef/knife/search.rb b/lib/chef/knife/search.rb
index 38d1ab3f42..d102c1e955 100644
--- a/lib/chef/knife/search.rb
+++ b/lib/chef/knife/search.rb
@@ -18,6 +18,7 @@
 
 require "chef/knife"
 require "chef/knife/core/node_presenter"
+require "addressable/uri"
 
 class Chef
   class Knife
@@ -85,8 +86,7 @@ class Chef
         end
 
         q = Chef::Search::Query.new
-        escaped_query = URI.escape(@query,
-                           Regexp.new("[^#{URI::PATTERN::UNRESERVED}]"))
+        escaped_query = Addressable::URI.encode_component(@query, Addressable::URI::CharacterClasses::QUERY)
 
         result_items = []
         result_count = 0
diff --git a/lib/chef/mixin/uris.rb b/lib/chef/mixin/uris.rb
index 24e8a4f9ed..7dc04d662b 100644
--- a/lib/chef/mixin/uris.rb
+++ b/lib/chef/mixin/uris.rb
@@ -17,6 +17,7 @@
 #
 
 require "uri"
+require "addressable/uri"
 
 class Chef
   module Mixin
@@ -34,7 +35,7 @@ class Chef
           URI.parse(source)
         rescue URI::InvalidURIError
           Chef::Log.warn("#{source} was an invalid URI. Trying to escape invalid characters")
-          URI.parse(URI.escape(source))
+          URI.parse(Addressable::URI.encode(source))
         end
       end
 
diff --git a/lib/chef/search/query.rb b/lib/chef/search/query.rb
index 024ec38a16..bea8205935 100644
--- a/lib/chef/search/query.rb
+++ b/lib/chef/search/query.rb
@@ -21,6 +21,7 @@ require "chef/exceptions"
 require "chef/server_api"
 
 require "uri"
+require "addressable/uri"
 
 class Chef
   class Search
@@ -134,15 +135,17 @@ WARNDEP
         args_h
       end
 
-      def escape(s)
-        s && URI.escape(s.to_s)
+      QUERY_PARAM_VALUE = Addressable::URI::CharacterClasses::QUERY + "\\&\\;"
+
+      def escape_value(s)
+        s && Addressable::URI.encode_component(s.to_s, QUERY_PARAM_VALUE)
       end
 
       def create_query_string(type, query, rows, start, sort)
-        qstr = "search/#{type}?q=#{escape(query)}"
-        qstr += "&sort=#{escape(sort)}" if sort
-        qstr += "&start=#{escape(start)}" if start
-        qstr += "&rows=#{escape(rows)}" if rows
+        qstr = "search/#{type}?q=#{escape_value(query)}"
+        qstr += "&sort=#{escape_value(sort)}" if sort
+        qstr += "&start=#{escape_value(start)}" if start
+        qstr += "&rows=#{escape_value(rows)}" if rows
         qstr
       end
 
diff --git a/spec/unit/provider/remote_file/local_file_spec.rb b/spec/unit/provider/remote_file/local_file_spec.rb
index 31f14fbe45..6f345cadd1 100644
--- a/spec/unit/provider/remote_file/local_file_spec.rb
+++ b/spec/unit/provider/remote_file/local_file_spec.rb
@@ -17,6 +17,8 @@
 #
 
 require "spec_helper"
+require "uri"
+require "addressable/uri"
 
 describe Chef::Provider::RemoteFile::LocalFile do
 
@@ -47,7 +49,7 @@ describe Chef::Provider::RemoteFile::LocalFile do
     end
 
     describe "when given local windows path with spaces" do
-      let(:uri) { URI.parse(URI.escape("file:///z:/windows/path/foo & bar.txt")) }
+      let(:uri) { URI.parse(Addressable::URI.encode("file:///z:/windows/path/foo & bar.txt")) }
       it "returns a valid windows local path" do
         expect(fetcher.source_path).to eq("z:/windows/path/foo & bar.txt")
       end
@@ -61,7 +63,7 @@ describe Chef::Provider::RemoteFile::LocalFile do
     end
 
     describe "when given unc windows path with spaces" do
-      let(:uri) { URI.parse(URI.escape("file:////server/share/windows/path/foo & bar.txt")) }
+      let(:uri) { URI.parse(Addressable::URI.encode("file:////server/share/windows/path/foo & bar.txt")) }
       it "returns a valid windows unc path" do
         expect(fetcher.source_path).to eq("//server/share/windows/path/foo & bar.txt")
       end