From 19cbd576a9594ea84a81d9f1af6bbdbae3be8fc5 Mon Sep 17 00:00:00 2001 From: Andy Brody Date: Sun, 5 Jun 2016 16:41:12 -0400 Subject: [PATCH] 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 | 8 ++ lib/restclient/request.rb | 200 +++++++++++++++++++++++++++++++------- spec/unit/request_spec.rb | 101 +++++++++++++++++-- 3 files changed, 265 insertions(+), 44 deletions(-) diff --git a/history.md b/history.md index 4a135bb..055b9cc 100644 --- a/history.md +++ b/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` diff --git a/lib/restclient/request.rb b/lib/restclient/request.rb index 86a3c6b..d82a639 100644 --- a/lib/restclient/request.rb +++ b/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, 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 - unless @cookies.empty? + # Render a hash of key => value pairs for cookies in the Request#cookie_jar + # 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 - # want to pass scary characters, just set the Cookie header directly. - # RFC6265 is actually much more restrictive than we are. - @cookies.each do |key, val| - unless valid_cookie_key?(key) - raise ArgumentError.new("Invalid cookie name: #{key.inspect}") - end - unless valid_cookie_value?(val) - raise ArgumentError.new("Invalid cookie value: #{val.inspect}") + @cookie_jar.cookies(uri).each do |c| + hash[c.name] = c.value + end + + hash + end + + # @return [HTTP::CookieJar] + 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::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 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 - user_headers[:cookie] = @cookies.map { |key, val| "#{key}=#{val}" }.sort.join('; ') + if key.is_a?(Symbol) + 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] 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`. # diff --git a/spec/unit/request_spec.rb b/spec/unit/request_spec.rb index d7789df..f643e05 100644 --- a/spec/unit/request_spec.rb +++ b/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')) - @request = RestClient::Request.new(:method => 'get', :url => 'example.com', :cookies => {:session_id => '1', :user_id => "someone" }) - @request.should_receive(:default_headers).and_return({'Foo' => 'bar'}) - @request.make_headers({}).should eq({ 'Foo' => 'bar', 'Cookie' => 'session_id=1; user_id=someone'}) + cookies_arr = [ + HTTP::Cookie.new('session_id', '1', domain: 'example.com', path: '/'), + HTTP::Cookie.new('user_id', 'someone', domain: 'example.com', path: '/'), + ] + + jar = HTTP::CookieJar.new + cookies_arr.each {|c| jar << c } + + # test Hash, HTTP::CookieJar, and Array 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