Implement cookie jar support for :cookies.

The `:cookies` option may now be:
- A hash of strings
- An array of HTTP::Cookie objects
- A full HTTP::CookieJar

Rewrite all of the cookie processing on RestClient::Request to operate
in terms of HTTP::CookieJar, and expose `Request#cookie_jar` and
`Request#make_cookie_header` methods.

Raise ArgumentError when `:cookies` is provided in both request headers
and request options. Warn when both `:cookies` and a `Cookie` header are
provided.

Fix up tests for the new functionality as well.

history.md

@@ -47,6 +47,14 @@ This release is largely API compatible, but makes several breaking changes.
 - Handle multiple HTTP response headers with the same name (except for
   Set-Cookie, which is special) by joining the values with a comma space,
   compliant with RFC 7230
+- Rewrite cookie support to be much smarter and to use cookie jars consistently
+  - The `:cookies` option may now be a Hash of Strings, an Array of
+    HTTP::Cookie objects, or a full HTTP::CookieJar.
+  - Add `RestClient::Request#cookie_jar` and reimplement `Request#cookies` to
+    be a wrapper around the cookie jar.
+  - Still support passing the `:cookies` option in the headers hash, but now
+    raise ArgumentError if that option is also passed to `Request#initialize`.
+  - Warn if both `:cookies` and a `Cookie` header are supplied.
 - Don't set basic auth header if explicit `Authorization` header is specified
 - Add `:proxy` option to requests, which can be used for thread-safe
   per-request proxy configuration, overriding `RestClient.proxy`

lib/restclient/request.rb

@@ -16,7 +16,9 @@ module RestClient
   # * :url
   # Optional parameters (have a look at ssl and/or uri for some explanations):
   # * :headers a hash containing the request headers
-  # * :cookies will replace possible cookies in the :headers
+  # * :cookies may be a Hash{String/Symbol => String} of cookie values, an
+  #     Array<HTTP::Cookie>, or an HTTP::CookieJar containing cookies. These
+  #     will be added to a cookie jar before the request is sent.
   # * :user and :password for basic auth, will be replaced by a user/password available in the :url
   # * :block_response call the provided block with the HTTPResponse as parameter
   # * :raw_response return a low-level RawResponse instead of a Response
@@ -38,7 +40,7 @@ module RestClient
   #      called with the HTTP request and request params.
   class Request
 
-    attr_reader :method, :uri, :url, :headers, :cookies, :payload, :proxy,
+    attr_reader :method, :uri, :url, :headers, :payload, :proxy,
                 :user, :password, :read_timeout, :max_redirects,
                 :open_timeout, :raw_response, :processed_headers, :args,
                 :ssl_opts
@@ -123,7 +125,10 @@ module RestClient
         raise ArgumentError, "must pass :url"
       end
       parse_url_with_auth!(url)
-      @cookies = @headers.delete(:cookies) || args[:cookies] || {}
+
+      # process cookie arguments found in headers or args
+      @cookie_jar = process_cookie_args!(@uri, @headers, args)
+
       @payload = Payload.generate(args[:payload])
       @user = args[:user]
       @password = args[:password]
@@ -267,49 +272,172 @@ module RestClient
       end
     end
 
-    def make_headers user_headers
+    # Render a hash of key => value pairs for cookies in the Request#cookie_jar
-      unless @cookies.empty?
+    # that are valid for the Request#uri. This will not necessarily include all
+    # cookies if there are duplicate keys. It's safer to use the cookie_jar
+    # directly if that's a concern.
+    #
+    # @see Request#cookie_jar
+    #
+    # @return [Hash]
+    #
+    def cookies
+      hash = {}
 
-        # Validate that the cookie names and values look sane. If you really
+      @cookie_jar.cookies(uri).each do |c|
-        # want to pass scary characters, just set the Cookie header directly.
+        hash[c.name] = c.value
-        # RFC6265 is actually much more restrictive than we are.
+      end
-        @cookies.each do |key, val|
+
-          unless valid_cookie_key?(key)
+      hash
-            raise ArgumentError.new("Invalid cookie name: #{key.inspect}")
+    end
-          end
+
-          unless valid_cookie_value?(val)
+    # @return [HTTP::CookieJar]
-            raise ArgumentError.new("Invalid cookie value: #{val.inspect}")
+    def cookie_jar
+      @cookie_jar
+    end
+
+    # Render a Cookie HTTP request header from the contents of the @cookie_jar,
+    # or nil if the jar is empty.
+    #
+    # @see Request#cookie_jar
+    #
+    # @return [String, nil]
+    #
+    def make_cookie_header
+      return nil if cookie_jar.nil?
+
+      arr = cookie_jar.cookies(url)
+      return nil if arr.empty?
+
+      return HTTP::Cookie.cookie_value(arr)
+    end
+
+    # Process cookies passed as hash or as HTTP::CookieJar. For backwards
+    # compatibility, these may be passed as a :cookies option masquerading
+    # inside the headers hash. To avoid confusion, if :cookies is passed in
+    # both headers and Request#initialize, raise an error.
+    #
+    # :cookies may be a:
+    # - Hash{String/Symbol => String}
+    # - Array<HTTP::Cookie>
+    # - HTTP::CookieJar
+    #
+    # Passing as a hash:
+    #   Keys may be symbols or strings. Values must be strings.
+    #   Infer the domain name from the request URI and allow subdomains (as
+    #   though '.example.com' had been set in a Set-Cookie header). Assume a
+    #   path of '/'.
+    #
+    #     RestClient::Request.new(url: 'http://example.com', method: :get,
+    #       :cookies => {:foo => 'Value', 'bar' => '123'}
+    #     )
+    #
+    # results in cookies as though set from the server by:
+    #     Set-Cookie: foo=Value; Domain=.example.com; Path=/
+    #     Set-Cookie: bar=123; Domain=.example.com; Path=/
+    #
+    # which yields a client cookie header of:
+    #     Cookie: foo=Value; bar=123
+    #
+    # Passing as HTTP::CookieJar, which will be passed through directly:
+    #
+    #     jar = HTTP::CookieJar.new
+    #     jar.add(HTTP::Cookie.new('foo', 'Value', domain: 'example.com',
+    #                              path: '/', for_domain: false))
+    #
+    #     RestClient::Request.new(..., :cookies => jar)
+    #
+    # @param [URI::HTTP] uri The URI for the request. This will be used to
+    # infer the domain name for cookies passed as strings in a hash. To avoid
+    # this implicit behavior, pass a full cookie jar or use HTTP::Cookie hash
+    # values.
+    # @param [Hash] headers The headers hash from which to pull the :cookies
+    #   option. MUTATION NOTE: This key will be deleted from the hash if
+    #   present.
+    # @param [Hash] args The options passed to Request#initialize. This hash
+    #   will be used as another potential source for the :cookies key.
+    #   These args will not be mutated.
+    #
+    # @return [HTTP::CookieJar] A cookie jar containing the parsed cookies.
+    #
+    def process_cookie_args!(uri, headers, args)
+
+      # Avoid ambiguity in whether options from headers or options from
+      # Request#initialize should take precedence by raising ArgumentError when
+      # both are present. Prior versions of rest-client claimed to give
+      # precedence to init options, but actually gave precedence to headers.
+      # Avoid that mess by erroring out instead.
+      if headers[:cookies] && args[:cookies]
+        raise ArgumentError.new(
+          "Cannot pass :cookies in Request.new() and in headers hash")
+      end
+
+      cookies_data = headers.delete(:cookies) || args[:cookies]
+
+      # return copy of cookie jar as is
+      if cookies_data.is_a?(HTTP::CookieJar)
+        return cookies_data.dup
+      end
+
+      # convert cookies hash into a CookieJar
+      jar = HTTP::CookieJar.new
+
+      (cookies_data || []).each do |key, val|
+
+        # Support for Array<HTTP::Cookie> mode:
+        # If key is a cookie object, add it to the jar directly and assert that
+        # there is no separate val.
+        if key.is_a?(HTTP::Cookie)
+          if val
+            raise ArgumentError.new("extra cookie val: #{val.inspect}")
           end
+
+          jar.add(key)
+          next
         end
 
-        user_headers = user_headers.dup
+        if key.is_a?(Symbol)
-        user_headers[:cookie] = @cookies.map { |key, val| "#{key}=#{val}" }.sort.join('; ')
+          key = key.to_s
+        end
+
+        # assume implicit domain from the request URI, and set for_domain to
+        # permit subdomains
+        jar.add(HTTP::Cookie.new(key, val, domain: uri.hostname.downcase,
+                                 path: '/', for_domain: true))
       end
+
+      jar
+    end
+
+    # Generate headers for use by a request. Header keys will be stringified
+    # using `#stringify_headers` to normalize them as capitalized strings.
+    #
+    # The final headers consist of:
+    #   - default headers from #default_headers
+    #   - user_headers provided here
+    #   - headers from the payload object (e.g. Content-Type, Content-Lenth)
+    #   - cookie headers from #make_cookie_header
+    #
+    # @param [Hash] user_headers User-provided headers to include
+    #
+    # @return [Hash<String, String>] A hash of HTTP headers => values
+    #
+    def make_headers(user_headers)
       headers = stringify_headers(default_headers).merge(stringify_headers(user_headers))
       headers.merge!(@payload.headers) if @payload
+
+      # merge in cookies
+      cookies = make_cookie_header
+      if cookies && !cookies.empty?
+        if headers['Cookie']
+          warn('warning: overriding "Cookie" header with :cookies option')
+        end
+        headers['Cookie'] = cookies
+      end
+
       headers
     end
 
-    # Do some sanity checks on cookie keys.
-    #
-    # Properly it should be a valid TOKEN per RFC 2616, but lots of servers are
-    # more liberal.
-    #
-    # Disallow the empty string as well as keys containing control characters,
-    # equals sign, semicolon, comma, or space.
-    #
-    def valid_cookie_key?(string)
-      return false if string.empty?
-
-      ! Regexp.new('[\x0-\x1f\x7f=;, ]').match(string)
-    end
-
-    # Validate cookie values. Rather than following RFC 6265, allow anything
-    # but control characters, comma, and semicolon.
-    def valid_cookie_value?(value)
-      ! Regexp.new('[\x0-\x1f\x7f,;]').match(value)
-    end
-
     # The proxy URI for this request. If `:proxy` was provided on this request,
     # use it over `RestClient.proxy`.
     #

spec/unit/request_spec.rb

@@ -126,10 +126,89 @@ describe RestClient::Request, :include_helpers do
   end
 
   it "correctly formats cookies provided to the constructor" do
-    URI.stub(:parse).and_return(double('uri', :user => nil, :password => nil, :hostname => 'example.com'))
+    cookies_arr = [
-    @request = RestClient::Request.new(:method => 'get', :url => 'example.com', :cookies => {:session_id => '1', :user_id => "someone" })
+      HTTP::Cookie.new('session_id', '1', domain: 'example.com', path: '/'),
-    @request.should_receive(:default_headers).and_return({'Foo' => 'bar'})
+      HTTP::Cookie.new('user_id', 'someone', domain: 'example.com', path: '/'),
-    @request.make_headers({}).should eq({ 'Foo' => 'bar', 'Cookie' => 'session_id=1; user_id=someone'})
+    ]
+
+    jar = HTTP::CookieJar.new
+    cookies_arr.each {|c| jar << c }
+
+    # test Hash, HTTP::CookieJar, and Array<HTTP::Cookie> modes
+    [
+      {session_id: '1', user_id: 'someone'},
+      jar,
+      cookies_arr
+    ].each do |cookies|
+      [true, false].each do |in_headers|
+        if in_headers
+          opts = {headers: {cookies: cookies}}
+        else
+          opts = {cookies: cookies}
+        end
+
+        request = RestClient::Request.new(method: :get, url: 'example.com', **opts)
+        request.should_receive(:default_headers).and_return({'Foo' => 'bar'})
+        request.make_headers({}).should eq({'Foo' => 'bar', 'Cookie' => 'session_id=1; user_id=someone'})
+        request.make_cookie_header.should eq 'session_id=1; user_id=someone'
+        request.cookies.should eq({'session_id' => '1', 'user_id' => 'someone'})
+        request.cookie_jar.cookies.length.should eq 2
+        request.cookie_jar.object_id.should_not eq jar.object_id # make sure we dup it
+      end
+    end
+
+    # test with no cookies
+    request = RestClient::Request.new(method: :get, url: 'example.com')
+    request.should_receive(:default_headers).and_return({'Foo' => 'bar'})
+    request.make_headers({}).should eq({'Foo' => 'bar'})
+    request.make_cookie_header.should be_nil
+    request.cookies.should eq({})
+    request.cookie_jar.cookies.length.should eq 0
+  end
+
+  it 'strips out cookies set for a different domain name' do
+    jar = HTTP::CookieJar.new
+    jar << HTTP::Cookie.new('session_id', '1', domain: 'other.example.com', path: '/')
+    jar << HTTP::Cookie.new('user_id', 'someone', domain: 'other.example.com', path: '/')
+
+    request = RestClient::Request.new(method: :get, url: 'www.example.com', cookies: jar)
+    request.should_receive(:default_headers).and_return({'Foo' => 'bar'})
+    request.make_headers({}).should eq({'Foo' => 'bar'})
+    request.make_cookie_header.should eq nil
+    request.cookies.should eq({})
+    request.cookie_jar.cookies.length.should eq 2
+  end
+
+  it 'assumes default domain and path for cookies set by hash' do
+    request = RestClient::Request.new(method: :get, url: 'www.example.com', cookies: {'session_id' => '1'})
+    request.cookie_jar.cookies.length.should eq 1
+
+    cookie = request.cookie_jar.cookies.first
+    cookie.should be_a(HTTP::Cookie)
+    cookie.domain.should eq('www.example.com')
+    cookie.for_domain?.should be_truthy
+    cookie.path.should eq('/')
+  end
+
+  it 'rejects or warns with contradictory cookie options' do
+    # same opt in two different places
+    lambda {
+      RestClient::Request.new(method: :get, url: 'example.com',
+                              cookies: {bar: '456'},
+                              headers: {cookies: {foo: '123'}})
+    }.should raise_error(ArgumentError, /Cannot pass :cookies in Request.*headers/)
+
+    # :cookies opt and Cookie header
+    [
+      {cookies: {foo: '123'}, headers: {cookie: 'foo'}},
+      {cookies: {foo: '123'}, headers: {'Cookie' => 'foo'}},
+      {headers: {cookies: {foo: '123'}, cookie: 'foo'}},
+      {headers: {cookies: {foo: '123'}, 'Cookie' => 'foo'}},
+    ].each do |opts|
+      fake_stderr {
+        RestClient::Request.new(method: :get, url: 'example.com', **opts)
+      }.should match(/warning: overriding "Cookie" header with :cookies option/)
+    end
   end
 
   it "does not escape or unescape cookies" do
@@ -147,23 +226,29 @@ describe RestClient::Request, :include_helpers do
     # Cookie validity is something of a mess, but we should reject the worst of
     # the RFC 6265 (4.1.1) prohibited characters such as control characters.
 
-    ['', 'foo=bar', 'foo;bar', "foo\nbar"].each do |cookie_name|
+    ['foo=bar', 'foo;bar', "foo\nbar"].each do |cookie_name|
       lambda {
         RestClient::Request.new(:method => 'get', :url => 'example.com',
                                 :cookies => {cookie_name => 'value'})
-      }.should raise_error(ArgumentError, /\AInvalid cookie name/)
+      }.should raise_error(ArgumentError, /\AInvalid cookie name/i)
     end
+
+    cookie_name = ''
+    lambda {
+      RestClient::Request.new(:method => 'get', :url => 'example.com',
+                              :cookies => {cookie_name => 'value'})
+    }.should raise_error(ArgumentError, /cookie name cannot be empty/i)
   end
 
   it "rejects cookie values containing invalid characters" do
     # Cookie validity is something of a mess, but we should reject the worst of
     # the RFC 6265 (4.1.1) prohibited characters such as control characters.
 
-    ['foo,bar', 'foo;bar', "foo\nbar"].each do |cookie_value|
+    ["foo\tbar", "foo\nbar"].each do |cookie_value|
       lambda {
         RestClient::Request.new(:method => 'get', :url => 'example.com',
                                 :cookies => {'test' => cookie_value})
-      }.should raise_error(ArgumentError, /\AInvalid cookie value/)
+      }.should raise_error(ArgumentError, /\AInvalid cookie value/i)
     end
   end