Merge tag 'v1.8.0' into merge-official

Version 1.8.0

* tag 'v1.8.0':
  Version 1.8.0
  Add history notes for 1.8.0.
  Update version spec test.
  Version 1.8.0.rc1
  Fix up cookie redirect functionality and tests.
  Add cookie jar for handling HTTP redirection.
  Add dependency on http-cookie gem.
  Tag version 1.7.3
  Import history from 2.0.x branch.
  Fix tests for 1.7.x backport of rest-client#352.
  Redact request password from logs.
  Drop MIME::Types monkey patch.
  Rename Platform.mac? to .mac_mri? for clarity.
  Fix RestClient::Request docs when used with yard

history.md

@@ -1,3 +1,21 @@
+# 1.8.0
+
+- Security: implement standards compliant cookie handling by adding a
+  dependency on http-cookie. This breaks compatibility, but was necessary to
+  address a session fixation / cookie disclosure vulnerability.
+  (#369 / CVE-2015-1820)
+
+  Previously, any Set-Cookie headers found in an HTTP 30x response would be
+  sent to the redirection target, regardless of domain. Responses now expose a
+  cookie jar and respect standards compliant domain / path flags in Set-Cookie
+  headers.
+
+# 1.7.3
+
+- Security: redact password in URI from logs (#349 / OSVDB-117461)
+- Drop monkey patch on MIME::Types (added `type_for_extension` method, use
+  the public interface instead.
+
 # 1.7.2
 
 - Ignore duplicate certificates in CA store on Windows

lib/restclient/abstract_response.rb

@@ -1,10 +1,11 @@
 require 'cgi'
+require 'http-cookie'
 
 module RestClient
 
   module AbstractResponse
 
-    attr_reader :net_http_res, :args
+    attr_reader :net_http_res, :args, :request
 
     # HTTP status code
     def code
@@ -22,11 +23,36 @@ def raw_headers
       @raw_headers ||= @net_http_res.to_hash
     end
 
+    def response_set_vars(net_http_res, args, request)
+      @net_http_res = net_http_res
+      @args = args
+      @request = request
+    end
+
     # Hash of cookies extracted from response headers
     def cookies
-      @cookies ||= (self.headers[:set_cookie] || {}).inject({}) do |out, cookie_content|
-        out.merge parse_cookie(cookie_content)
+      hash = {}
+
+      cookie_jar.cookies.each do |cookie|
+        hash[cookie.name] = cookie.value
       end
+
+      hash
+    end
+
+    # Cookie jar extracted from response headers.
+    #
+    # @return [HTTP::CookieJar]
+    #
+    def cookie_jar
+      return @cookie_jar if @cookie_jar
+
+      jar = HTTP::CookieJar.new
+      headers.fetch(:set_cookie, []).each do |cookie|
+        jar.parse(cookie, @request.url)
+      end
+
+      @cookie_jar = jar
     end
 
     # Return the default behavior corresponding to the response code:
@@ -61,25 +87,28 @@ def description
 
     # Follow a redirection
     def follow_redirection request = nil, result = nil, & block
+      new_args = @args.dup
+
       url = headers[:location]
       if url !~ /^http/
-        url = URI.parse(args[:url]).merge(url).to_s
+        url = URI.parse(request.url).merge(url).to_s
       end
-      args[:url] = url
+      new_args[:url] = url
       if request
         if request.max_redirects == 0
           raise MaxRedirectsReached
         end
-        args[:password] = request.password
-        args[:user] = request.user
-        args[:headers] = request.headers
-        args[:max_redirects] = request.max_redirects - 1
-        # pass any cookie set in the result
-        if result && result['set-cookie']
-          args[:headers][:cookies] = (args[:headers][:cookies] || {}).merge(parse_cookie(result['set-cookie']))
-        end
+        new_args[:password] = request.password
+        new_args[:user] = request.user
+        new_args[:headers] = request.headers
+        new_args[:max_redirects] = request.max_redirects - 1
+
+        # TODO: figure out what to do with original :cookie, :cookies values
+        new_args[:headers]['Cookie'] = HTTP::Cookie.cookie_value(
+          cookie_jar.cookies(new_args.fetch(:url)))
       end
-      Request.execute args, &block
+
+      Request.execute(new_args, &block)
     end
 
     def self.beautify_headers(headers)

lib/restclient/exceptions.rb

@@ -195,8 +195,8 @@ def initialize(message)
   end
 end
 
-# backwards compatibility
 class RestClient::Request
+  # backwards compatibility
   Redirect = RestClient::Redirect
   Unauthorized = RestClient::Unauthorized
   RequestFailed = RestClient::RequestFailed

lib/restclient/platform.rb

@@ -4,7 +4,7 @@ module Platform
     # be false for jruby even on OS X.
     #
     # @return [Boolean]
-    def self.mac?
+    def self.mac_mri?
       RUBY_PLATFORM.include?('darwin')
     end
 

lib/restclient/raw_response.rb

@@ -13,12 +13,13 @@ class RawResponse
 
     include AbstractResponse
 
-    attr_reader :file
+    attr_reader :file, :request
 
-    def initialize tempfile, net_http_res, args
+    def initialize(tempfile, net_http_res, args, request)
       @net_http_res = net_http_res
       @args = args
       @file = tempfile
+      @request = request
     end
 
     def to_s

lib/restclient/request.rb

@@ -358,7 +358,7 @@ def self.default_ssl_cert_store
 
     def print_verify_callback_warnings
       warned = false
-      if RestClient::Platform.mac?
+      if RestClient::Platform.mac_mri?
         warn('warning: ssl_verify_callback return code is ignored on OS X')
         warned = true
       end
@@ -508,9 +508,9 @@ def fetch_body(http_response)
     def process_result res, & block
       if @raw_response
         # We don't decode raw requests
-        response = RawResponse.new(@tf, res, args)
+        response = RawResponse.new(@tf, res, args, self)
       else
-        response = Response.create(Request.decode(res['content-encoding'], res.body), res, args)
+        response = Response.create(Request.decode(res['content-encoding'], res.body), res, args, self)
       end
 
       if block_given?
@@ -543,6 +543,17 @@ def log_request
       return unless RestClient.log
 
       out = []
+      sanitized_url = begin
+        uri = URI.parse(url)
+        uri.password = "REDACTED" if uri.password
+        uri.to_s
+      rescue URI::InvalidURIError
+        # An attacker may be able to manipulate the URL to be
+        # invalid, which could force discloure of a password if
+        # we show any of the un-parsed URL here.
+        "[invalid uri]"
+      end
+
       out << "RestClient.#{method} #{url.inspect}"
       out << payload.short_inspect(RestClient.log_filters) if payload
       out << processed_headers.to_a.sort.map { |(k, v)| [k.inspect, v.inspect].join("=>") }.join(", ")
@@ -577,16 +588,17 @@ def stringify_headers headers
           key = key.to_s.split(/_/).map { |w| w.capitalize }.join('-')
         end
         if 'CONTENT-TYPE' == key.upcase
-          target_value = value.to_s
-          result[key] = MIME::Types.type_for_extension target_value
+          result[key] = maybe_convert_extension(value.to_s)
         elsif 'ACCEPT' == key.upcase
           # Accept can be composed of several comma-separated values
           if value.is_a? Array
             target_values = value
           else
             target_values = value.to_s.split ','
           end
-          result[key] = target_values.map { |ext| MIME::Types.type_for_extension(ext.to_s.strip) }.join(', ')
+          result[key] = target_values.map { |ext|
+            maybe_convert_extension(ext.to_s.strip)
+          }.join(', ')
         else
           result[key] = value.to_s
         end
@@ -604,6 +616,7 @@ def parser
       URI.const_defined?(:Parser) ? URI::Parser.new : URI
     end
 
+<<<<<<< HEAD
     def apply_log_filters(message, filters)
       return message  unless message
       filters.reduce(message) do |current, filter|
@@ -622,10 +635,40 @@ def type_for_extension ext
       candidates = @extension_index[ext]
       candidates.empty? ? ext : candidates[0].content_type
     end
-
-    class << self
-      def type_for_extension ext
-        @__types__.type_for_extension ext
+=======
+    # Given a MIME type or file extension, return either a MIME type or, if
+    # none is found, the input unchanged.
+    #
+    #     >> maybe_convert_extension('json')
+    #     => 'application/json'
+    #
+    #     >> maybe_convert_extension('unknown')
+    #     => 'unknown'
+    #
+    #     >> maybe_convert_extension('application/xml')
+    #     => 'application/xml'
+    #
+    # @param ext [String]
+    #
+    # @return [String]
+    #
+    def maybe_convert_extension(ext)
+      unless ext =~ /\A[a-zA-Z0-9_@-]+\z/
+        # Don't look up strings unless they look like they could be a file
+        # extension known to mime-types.
+        #
+        # There currently isn't any API public way to look up extensions
+        # directly out of MIME::Types, but the type_for() method only strips
+        # off after a period anyway.
+        return ext
+      end
+>>>>>>> v1.8.0
+
+      types = MIME::Types.type_for(ext)
+      if types.empty?
+        ext
+      else
+        types.first.content_type
       end
     end
   end

lib/restclient/response.rb

@@ -6,17 +6,14 @@ module Response
 
     include AbstractResponse
 
-    attr_accessor :args, :net_http_res
-
     def body
       self
     end
 
-    def self.create body, net_http_res, args
+    def self.create body, net_http_res, args, request
       result = body || ''
       result.extend Response
-      result.net_http_res = net_http_res
-      result.args = args
+      result.response_set_vars(net_http_res, args, request)
       result
     end
 

lib/restclient/version.rb

@@ -1,5 +1,5 @@
 module RestClient
-  VERSION = '1.7.2' unless defined?(self::VERSION)
+  VERSION = '1.8.0' unless defined?(self::VERSION)
 
   def self.version
     VERSION

rest-client.gemspec

@@ -22,6 +22,7 @@ Gem::Specification.new do |s|
   s.add_development_dependency('pry-doc')
   s.add_development_dependency('rdoc', '>= 2.4.2', '< 5.0')
 
+  s.add_dependency('http-cookie', '>= 1.0.2', '< 2.0')
   s.add_dependency('mime-types', '>= 1.16', '< 3.0')
   s.add_dependency('netrc', '~> 0.7')
 

spec/integration/request_spec.rb

@@ -34,7 +34,7 @@
     #
     # On OS X, this test fails since Apple has patched OpenSSL to always fall
     # back on the system CA store.
-    it "is unsuccessful with an incorrect ca_file", :unless => RestClient::Platform.mac? do
+    it "is unsuccessful with an incorrect ca_file", :unless => RestClient::Platform.mac_mri? do
       request = RestClient::Request.new(
         :method => :get,
         :url => 'https://www.mozilla.org',
@@ -45,7 +45,7 @@
 
     # On OS X, this test fails since Apple has patched OpenSSL to always fall
     # back on the system CA store.
-    it "is unsuccessful with an incorrect ca_path", :unless => RestClient::Platform.mac? do
+    it "is unsuccessful with an incorrect ca_path", :unless => RestClient::Platform.mac_mri? do
       request = RestClient::Request.new(
         :method => :get,
         :url => 'https://www.mozilla.org',
@@ -79,7 +79,7 @@
     end
 
     it "fails verification when the callback returns false",
-       :unless => RestClient::Platform.mac? do
+       :unless => RestClient::Platform.mac_mri? do
       request = RestClient::Request.new(
         :method => :get,
         :url => 'https://www.mozilla.org',
@@ -90,7 +90,7 @@
     end
 
     it "succeeds verification when the callback returns true",
-       :unless => RestClient::Platform.mac? do
+       :unless => RestClient::Platform.mac_mri? do
       request = RestClient::Request.new(
         :method => :get,
         :url => 'https://www.mozilla.org',

spec/unit/abstract_response_spec.rb

@@ -8,16 +8,18 @@ class MyAbstractResponse
 
     attr_accessor :size
 
-    def initialize net_http_res, args
+    def initialize net_http_res, args, request
       @net_http_res = net_http_res
       @args = args
+      @request = request
     end
 
   end
 
   before do
     @net_http_res = double('net http response')
-    @response = MyAbstractResponse.new(@net_http_res, {})
+    @request = double('restclient request', :url => 'http://example.com')
+    @response = MyAbstractResponse.new(@net_http_res, {}, @request)
   end
 
   it "fetches the numeric response code" do
@@ -53,7 +55,8 @@ def initialize net_http_res, args
 
   it "extract strange cookies" do
     @net_http_res.should_receive(:to_hash).and_return('set-cookie' => ['session_id=ZJ/HQVH6YE+rVkTpn0zvTQ==; path=/'])
-    @response.cookies.should eq({ 'session_id' => 'ZJ%2FHQVH6YE+rVkTpn0zvTQ%3D%3D' })
+    @response.headers.should eq({:set_cookie => ['session_id=ZJ/HQVH6YE+rVkTpn0zvTQ==; path=/']})
+    @response.cookies.should eq({ 'session_id' => 'ZJ/HQVH6YE+rVkTpn0zvTQ==' })
   end
 
   it "doesn't escape cookies" do

spec/unit/raw_response_spec.rb

@@ -4,7 +4,8 @@
   before do
     @tf = double("Tempfile", :read => "the answer is 42", :open => true)
     @net_http_res = double('net http response')
-    @response = RestClient::RawResponse.new(@tf, @net_http_res, {})
+    @request = double('http request')
+    @response = RestClient::RawResponse.new(@tf, @net_http_res, {}, @request)
   end
 
   it "behaves like string" do

spec/unit/request_spec.rb

@@ -527,6 +527,18 @@
         log[1].should eq %Q{# => {"some": "***"}}
       end
     end
+
+    it 'does not log request password' do
+      log = RestClient.log = []
+      RestClient::Request.new(:method => :get, :url => 'http://user:password@url', :headers => {:user_agent => 'rest-client', :accept => '*/*'}).log_request
+      log[0].should eq %Q{RestClient.get "http://user:REDACTED@url", "Accept"=>"*/*", "Accept-Encoding"=>"gzip, deflate", "User-Agent"=>"rest-client"\n}
+    end
+
+    it 'logs invalid URIs, even though they will fail elsewhere' do
+      log = RestClient.log = []
+      RestClient::Request.new(:method => :get, :url => 'http://a@b:c', :headers => {:user_agent => 'rest-client', :accept => '*/*'}).log_request
+      log[0].should eq %Q{RestClient.get "[invalid uri]", "Accept"=>"*/*", "Accept-Encoding"=>"gzip, deflate", "User-Agent"=>"rest-client"\n}
+    end
   end
 
   it "strips the charset from the response content type" do