diff --git a/lib/restclient/request.rb b/lib/restclient/request.rb index 7dedf77..0290379 100644 --- a/lib/restclient/request.rb +++ b/lib/restclient/request.rb @@ -133,6 +133,7 @@ module RestClient def initialize( method:, url:, + payload: nil, headers: {}, params: {}, cookies: nil, @@ -145,7 +146,7 @@ module RestClient read_timeout: :notprovided, open_timeout: :notprovided, timeout: :notprovided, ssl_client_cert: nil, ssl_client_key: nil, ssl_ca_file: nil, - ssl_ca_path: nil, ssl_cert_store: nil, ssl_verify_callback: + ssl_ca_path: nil, ssl_cert_store: :notprovided, ssl_verify_callback: nil, ssl_verify_callback_warnings: nil, ssl_version: nil, ssl_ciphers: nil, before_execution_proc: nil) @@ -172,25 +173,20 @@ module RestClient end @method = normalize_method(method) - @headers = headers.dup.freeze # TODO don't dup/freeze + @headers = headers.dup.freeze @url = process_url_params(normalize_url(url), params) @user = @password = nil - parse_url_with_auth!(url, use_netrc: use_netrc) + parse_url_with_auth!(@url, use_netrc: use_netrc) # process cookies - if cookies - @cookie_jar = process_cookie_args(@uri, cookies) - else - @cookie_jar = nil - end + @cookie_jar = process_cookie_args(@uri, cookies) @payload = Payload.generate(payload) @user = user if user @password = password if password - @read_timeout = @open_timeout = nil if timeout != :notprovided @read_timeout = @open_timeout = timeout end @@ -226,23 +222,23 @@ module RestClient @ssl_client_key = ssl_client_key @ssl_ca_file = ssl_ca_file @ssl_ca_path = ssl_ca_path - @ssl_cert_store = ssl_cert_store + @ssl_cert_store = ssl_cert_store if ssl_cert_store != :notprovided @ssl_version = ssl_version @ssl_ciphers = ssl_ciphers @ssl_verify_callback = ssl_verify_callback @ssl_verify_callback_warnings = ssl_verify_callback_warnings # If there's no CA file, CA path, or cert store provided, use default - if !ssl_ca_file && !ssl_ca_path && !ssl_cert_store + if !@ssl_ca_file && !@ssl_ca_path && !defined?(@ssl_cert_store) @ssl_cert_store = self.class.default_ssl_cert_store end - unless ssl_ciphers + unless @ssl_ciphers # If we're on a Ruby version that has insecure default ciphers, # override it with our default list. if WeakDefaultCiphers.include?( OpenSSL::SSL::SSLContext::DEFAULT_PARAMS.fetch(:ciphers)) - @ssl_opts[:ciphers] = DefaultCiphers + @ssl_ciphers = DefaultCiphers end end end @@ -287,7 +283,8 @@ module RestClient when RestClient::ParamsArray else raise ArgumentError.new( - 'params must be Hash or RestClient::ParamsArray') + 'params must be Hash or RestClient::ParamsArray, got ' + + url_params.inspect) end # build resulting URL with query string @@ -318,8 +315,6 @@ module RestClient def cookies hash = {} - return nil unless @cookie_jar - @cookie_jar.cookies(uri).each do |c| hash[c.name] = c.value end @@ -748,14 +743,8 @@ module RestClient warn('Try passing :verify_ssl => false instead.') end - net.read_timeout = @read_timeout - if defined? @open_timeout - if @open_timeout == -1 - warn 'Deprecated: to disable timeouts, please use nil instead of -1' - @open_timeout = nil - end - net.open_timeout = @open_timeout - end + net.read_timeout = @read_timeout if defined?(@read_timeout) + net.open_timeout = @open_timeout if defined?(@open_timeout) RestClient.before_execution_procs.each do |before_proc| before_proc.call(req, self) @@ -796,7 +785,7 @@ module RestClient end def setup_credentials(req) - req.basic_auth(user, password) if user && !headers.has_key?("Authorization") + req.basic_auth(user, password) if user && !@processed_headers.has_key?("Authorization") end def fetch_body(http_response) diff --git a/spec/helpers.rb b/spec/helpers.rb index 1de717d..709e88d 100644 --- a/spec/helpers.rb +++ b/spec/helpers.rb @@ -17,6 +17,6 @@ module Helpers def request_double(url: 'http://example.com', method: 'get') double('request', url: url, uri: URI.parse(url), method: method, user: nil, password: nil, cookie_jar: HTTP::CookieJar.new, - redirection_history: nil, args: {url: url, method: method}) + redirection_history: nil, original_opts: {url: url, method: method}) end end diff --git a/spec/integration/request_spec.rb b/spec/integration/request_spec.rb index 99bfefa..fa8c65c 100644 --- a/spec/integration/request_spec.rb +++ b/spec/integration/request_spec.rb @@ -28,10 +28,6 @@ describe RestClient::Request do expect { request.execute }.to_not raise_error end - # TODO: deprecate and remove RestClient::SSLCertificateNotVerified and just - # pass through OpenSSL::SSL::SSLError directly. See note in - # lib/restclient/request.rb. - # # 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_mri? do @@ -40,7 +36,7 @@ describe RestClient::Request do :url => 'https://www.mozilla.org', :ssl_ca_file => File.join(File.dirname(__FILE__), "certs", "verisign.crt") ) - expect { request.execute }.to raise_error(RestClient::SSLCertificateNotVerified) + expect { request.execute }.to raise_error(OpenSSL::SSL::SSLError, /certificate verify failed/) end # On OS X, this test fails since Apple has patched OpenSSL to always fall @@ -51,7 +47,7 @@ describe RestClient::Request do :url => 'https://www.mozilla.org', :ssl_ca_path => File.join(File.dirname(__FILE__), "capath_verisign") ) - expect { request.execute }.to raise_error(RestClient::SSLCertificateNotVerified) + expect { request.execute }.to raise_error(OpenSSL::SSL::SSLError, /certificate verify failed/) end it "is successful using the default system cert store" do @@ -86,7 +82,7 @@ describe RestClient::Request do :verify_ssl => true, :ssl_verify_callback => lambda { |preverify_ok, store_ctx| false }, ) - expect { request.execute }.to raise_error(RestClient::SSLCertificateNotVerified) + expect { request.execute }.to raise_error(OpenSSL::SSL::SSLError, /certificate verify failed/) end it "succeeds verification when the callback returns true", diff --git a/spec/unit/request2_spec.rb b/spec/unit/request2_spec.rb index 71e5d6e..eae4356 100644 --- a/spec/unit/request2_spec.rb +++ b/spec/unit/request2_spec.rb @@ -5,29 +5,26 @@ describe RestClient::Request do context 'params for GET requests' do it "manage params for get requests" do stub_request(:get, 'http://some/resource?a=b&c=d').with(:headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip, deflate', 'Foo'=>'bar'}).to_return(:body => 'foo', :status => 200) - expect(RestClient::Request.execute(:url => 'http://some/resource', :method => :get, :headers => {:foo => :bar, :params => {:a => :b, 'c' => 'd'}}).body).to eq 'foo' - - stub_request(:get, 'http://some/resource').with(:headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip, deflate', 'Foo'=>'bar'}).to_return(:body => 'foo', :status => 200) - expect(RestClient::Request.execute(:url => 'http://some/resource', :method => :get, :headers => {:foo => :bar, :params => :a}).body).to eq 'foo' + expect(RestClient::Request.execute(:url => 'http://some/resource', :method => :get, :headers => {:foo => :bar}, :params => {:a => :b, 'c' => 'd'}).body).to eq 'foo' end it 'adds GET params when params are present in URL' do stub_request(:get, 'http://some/resource?a=b&c=d').with(:headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip, deflate', 'Foo'=>'bar'}).to_return(:body => 'foo', :status => 200) - expect(RestClient::Request.execute(:url => 'http://some/resource?a=b', :method => :get, :headers => {:foo => :bar, :params => {:c => 'd'}}).body).to eq 'foo' + expect(RestClient::Request.execute(:url => 'http://some/resource?a=b', :method => :get, :headers => {:foo => :bar}, :params => {:c => 'd'}).body).to eq 'foo' end it 'encodes nested GET params' do stub_request(:get, 'http://some/resource?a[foo][]=1&a[foo][]=2&a[bar]&b=foo+bar&math=2+%2B+2+%3D%3D+4').with(:headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip, deflate'}).to_return(:body => 'foo', :status => 200) - expect(RestClient::Request.execute(url: 'http://some/resource', method: :get, headers: { - params: { - a: { - foo: [1,2], - bar: nil, - }, - b: 'foo bar', - math: '2 + 2 == 4', - } - }).body).to eq 'foo' + expect(RestClient::Request.execute(url: 'http://some/resource', method: :get, + params: { + a: { + foo: [1,2], + bar: nil, + }, + b: 'foo bar', + math: '2 + 2 == 4', + } + ).body).to eq 'foo' end end @@ -38,7 +35,7 @@ describe RestClient::Request do response_value = http_response.body end stub_request(:get, 'http://some/resource?a=b&c=d').with(:headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip, deflate', 'Foo'=>'bar'}).to_return(:body => 'foo', :status => 200) - RestClient::Request.execute(:url => 'http://some/resource', :method => :get, :headers => {:foo => :bar, :params => {:a => :b, 'c' => 'd'}}, :block_response => block) + RestClient::Request.execute(:url => 'http://some/resource', :method => :get, :headers => {:foo => :bar}, :params => {:a => :b, 'c' => 'd'}, :block_response => block) expect(response_value).to eq "foo" end diff --git a/spec/unit/request_spec.rb b/spec/unit/request_spec.rb index b338947..b5a2a9c 100644 --- a/spec/unit/request_spec.rb +++ b/spec/unit/request_spec.rb @@ -148,21 +148,13 @@ describe RestClient::Request, :include_helpers do 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) - expect(request).to receive(:default_headers).and_return({'Foo' => 'bar'}) - expect(request.make_headers({})).to eq({'Foo' => 'bar', 'Cookie' => 'session_id=1; user_id=someone'}) - expect(request.make_cookie_header).to eq 'session_id=1; user_id=someone' - expect(request.cookies).to eq({'session_id' => '1', 'user_id' => 'someone'}) - expect(request.cookie_jar.cookies.length).to eq 2 - expect(request.cookie_jar.object_id).not_to eq jar.object_id # make sure we dup it - end + request = RestClient::Request.new(method: :get, url: 'example.com', cookies: cookies) + expect(request).to receive(:default_headers).and_return({'Foo' => 'bar'}) + expect(request.make_headers({})).to eq({'Foo' => 'bar', 'Cookie' => 'session_id=1; user_id=someone'}) + expect(request.make_cookie_header).to eq 'session_id=1; user_id=someone' + expect(request.cookies).to eq({'session_id' => '1', 'user_id' => 'someone'}) + expect(request.cookie_jar.cookies.length).to eq 2 + expect(request.cookie_jar.object_id).not_to eq jar.object_id # make sure we dup it end # test with no cookies @@ -199,19 +191,10 @@ describe RestClient::Request, :include_helpers do end it 'rejects or warns with contradictory cookie options' do - # same opt in two different places - expect { - RestClient::Request.new(method: :get, url: 'example.com', - cookies: {bar: '456'}, - headers: {cookies: {foo: '123'}}) - }.to 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| expect(fake_stderr { RestClient::Request.new(method: :get, url: 'example.com', **opts) @@ -437,9 +420,9 @@ describe RestClient::Request, :include_helpers do end it "does not attempt to send credentials if Authorization header is set" do - @request.headers['Authorization'] = 'Token abc123' - allow(@request).to receive(:user).and_return('joe') - allow(@request).to receive(:password).and_return('mypass') + @request = RestClient::Request.new(method: :get, url: 'http://some/resource', + headers: {authorization: 'Token abc123'}, + user: 'joe', password: 'mypass') req = double("request") expect(req).not_to receive(:basic_auth) @request.send(:setup_credentials, req) @@ -740,34 +723,6 @@ describe RestClient::Request, :include_helpers do @request.send(:transmit, @uri, 'req', nil) end - - it 'deprecated: warns when disabling timeout by setting it to -1' do - @request = RestClient::Request.new(:method => :put, :url => 'http://some/resource', :payload => 'payload', :read_timeout => -1) - allow(@http).to receive(:request) - allow(@request).to receive(:process_result) - allow(@request).to receive(:response_log) - - expect(@net).to receive(:read_timeout=).with(nil) - - expect(fake_stderr { - @request.send(:transmit, @uri, 'req', nil) - }).to match(/^Deprecated: .*timeout.* nil instead of -1$/) - end - - it "deprecated: disable timeout by setting it to -1" do - @request = RestClient::Request.new(:method => :put, :url => 'http://some/resource', :payload => 'payload', :read_timeout => -1, :open_timeout => -1) - allow(@http).to receive(:request) - allow(@request).to receive(:process_result) - allow(@request).to receive(:response_log) - - expect(@request).to receive(:warn) - expect(@net).to receive(:read_timeout=).with(nil) - - expect(@request).to receive(:warn) - expect(@net).to receive(:open_timeout=).with(nil) - - @request.send(:transmit, @uri, 'req', nil) - end end describe "ssl" do @@ -1228,24 +1183,24 @@ describe RestClient::Request, :include_helpers do describe 'process_url_params' do it 'should handle basic URL params' do - expect(@request.process_url_params('https://example.com/foo', params: {key1: 123, key2: 'abc'})). + expect(@request.process_url_params('https://example.com/foo', {key1: 123, key2: 'abc'})). to eq 'https://example.com/foo?key1=123&key2=abc' - expect(@request.process_url_params('https://example.com/foo', params: {'key1' => 123})). + expect(@request.process_url_params('https://example.com/foo', {'key1' => 123})). to eq 'https://example.com/foo?key1=123' expect(@request.process_url_params('https://example.com/path', - params: {foo: 'one two', bar: 'three + four == seven'})). + {foo: 'one two', bar: 'three + four == seven'})). to eq 'https://example.com/path?foo=one+two&bar=three+%2B+four+%3D%3D+seven' end it 'should combine with & when URL params already exist' do - expect(@request.process_url_params('https://example.com/path?foo=1', params: {bar: 2})). + expect(@request.process_url_params('https://example.com/path?foo=1', {bar: 2})). to eq 'https://example.com/path?foo=1&bar=2' end it 'should handle complex nested URL params per Rack / Rails conventions' do - expect(@request.process_url_params('https://example.com/', params: { + expect(@request.process_url_params('https://example.com/', { foo: [1,2,3], null: nil, false: false, @@ -1258,7 +1213,7 @@ describe RestClient::Request, :include_helpers do it 'should handle ParamsArray objects' do expect(@request.process_url_params('https://example.com/', - params: RestClient::ParamsArray.new([[:foo, 1], [:foo, 2]]) + RestClient::ParamsArray.new([[:foo, 1], [:foo, 2]]) )).to eq 'https://example.com/?foo=1&foo=2' end end diff --git a/spec/unit/restclient_spec.rb b/spec/unit/restclient_spec.rb index cb4dfe0..b942b80 100644 --- a/spec/unit/restclient_spec.rb +++ b/spec/unit/restclient_spec.rb @@ -3,37 +3,37 @@ require_relative '_lib' describe RestClient do describe "API" do it "GET" do - expect(RestClient::Request).to receive(:execute).with(:method => :get, :url => 'http://some/resource', :headers => {}) + expect(RestClient::Request).to receive(:execute).with(:method => :get, :url => 'http://some/resource') RestClient.get('http://some/resource') end it "POST" do - expect(RestClient::Request).to receive(:execute).with(:method => :post, :url => 'http://some/resource', :payload => 'payload', :headers => {}) + expect(RestClient::Request).to receive(:execute).with(:method => :post, :url => 'http://some/resource', :payload => 'payload') RestClient.post('http://some/resource', 'payload') end it "PUT" do - expect(RestClient::Request).to receive(:execute).with(:method => :put, :url => 'http://some/resource', :payload => 'payload', :headers => {}) + expect(RestClient::Request).to receive(:execute).with(:method => :put, :url => 'http://some/resource', :payload => 'payload') RestClient.put('http://some/resource', 'payload') end it "PATCH" do - expect(RestClient::Request).to receive(:execute).with(:method => :patch, :url => 'http://some/resource', :payload => 'payload', :headers => {}) + expect(RestClient::Request).to receive(:execute).with(:method => :patch, :url => 'http://some/resource', :payload => 'payload') RestClient.patch('http://some/resource', 'payload') end it "DELETE" do - expect(RestClient::Request).to receive(:execute).with(:method => :delete, :url => 'http://some/resource', :headers => {}) + expect(RestClient::Request).to receive(:execute).with(:method => :delete, :url => 'http://some/resource') RestClient.delete('http://some/resource') end it "HEAD" do - expect(RestClient::Request).to receive(:execute).with(:method => :head, :url => 'http://some/resource', :headers => {}) + expect(RestClient::Request).to receive(:execute).with(:method => :head, :url => 'http://some/resource') RestClient.head('http://some/resource') end it "OPTIONS" do - expect(RestClient::Request).to receive(:execute).with(:method => :options, :url => 'http://some/resource', :headers => {}) + expect(RestClient::Request).to receive(:execute).with(:method => :options, :url => 'http://some/resource') RestClient.options('http://some/resource') end end