Refactor URI parsing to happen earlier.

- Move URI parsing into Request#initialize, rather than checking for
  validity in Request#transmit. We should fail fast when we already know
  we have a completely invalid URI.
- Add a `!` to parse_url_with_auth because it is otherwise surprising
  that it operates by side effect.
- Don't set several SSL related options unless we have an HTTPS URI.

Fixes: #302

history.md

@@ -65,6 +65,8 @@ This release is largely API compatible, but makes several breaking changes.
   possible to add procs like `RestClient.add_before_execution_proc` to a single
   request without global state.
 - Run tests on Travis's beta OS X support.
+- Make `Request#transmit` a private method, along with a few others.
+- Refactor URI parsing to happen earlier, in Request initialization.
 
 # 2.0.0.rc1
 

lib/restclient/request.rb

@@ -40,7 +40,7 @@ module RestClient
 
     # TODO: rename timeout to read_timeout
 
-    attr_reader :method, :url, :headers, :cookies, :payload, :proxy,
+    attr_reader :method, :uri, :url, :headers, :cookies, :payload, :proxy,
                 :user, :password, :read_timeout, :max_redirects,
                 :open_timeout, :raw_response, :processed_headers, :args,
                 :ssl_opts
@@ -124,6 +124,7 @@ module RestClient
       else
         raise ArgumentError, "must pass :url"
       end
+      parse_url_with_auth!(url)
       @cookies = @headers.delete(:cookies) || args[:cookies] || {}
       @payload = Payload.generate(args[:payload])
       @user = args[:user]
@@ -171,17 +172,21 @@ module RestClient
         end
       end
 
-      # If there's no CA file, CA path, or cert store provided, use default
-      if !ssl_ca_file && !ssl_ca_path && !@ssl_opts.include?(:cert_store)
-        @ssl_opts[:cert_store] = self.class.default_ssl_cert_store
-      end
+      # Set some other default SSL options, but only if we have an HTTPS URI.
+      if use_ssl?
 
-      unless @ssl_opts.include?(: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
+        # If there's no CA file, CA path, or cert store provided, use default
+        if !ssl_ca_file && !ssl_ca_path && !@ssl_opts.include?(:cert_store)
+          @ssl_opts[:cert_store] = self.class.default_ssl_cert_store
+        end
+
+        unless @ssl_opts.include?(: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
+          end
         end
       end
 
@@ -194,8 +199,6 @@ module RestClient
     end
 
     def execute & block
-      uri = parse_url_with_auth(url)
-
       # With 2.0.0+, net/http accepts URI objects in requests and handles wrapping
       # IPv6 addresses in [] for use in the Host request header.
       request_uri = RUBY_VERSION >= "2.0.0" ? uri : uri.request_uri
@@ -214,6 +217,14 @@ module RestClient
       end
     end
 
+    # Return true if the request URI will use HTTPS.
+    #
+    # @return [Boolean]
+    #
+    def use_ssl?
+      uri.is_a?(URI::HTTPS)
+    end
+
     # Extract the query parameters and append them to the url
     def process_url_params url, headers
       url_params = {}
@@ -349,16 +360,6 @@ module RestClient
       URI.parse(url)
     end
 
-    def parse_url_with_auth(url)
-      uri = parse_url(url)
-      @user = CGI.unescape(uri.user) if uri.user
-      @password = CGI.unescape(uri.password) if uri.password
-      if !@user && !@password
-        @user, @password = Netrc.read[uri.hostname]
-      end
-      uri
-    end
-
     def process_payload(p=nil, parent_key=nil)
       unless p.is_a?(Hash)
         p
@@ -426,18 +427,16 @@ module RestClient
     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]"
+      if uri.password
+        sanitized_uri = uri.dup
+        sanitized_uri.password = "REDACTED"
+        sanitized_url = sanitized_uri.to_s
+      else
+        sanitized_url = uri.to_s
       end
 
+      out = []
+
       out << "RestClient.#{method} #{sanitized_url.inspect}"
       out << payload.short_inspect if payload
       out << processed_headers.to_a.sort.map { |(k, v)| [k.inspect, v.inspect].join("=>") }.join(", ")
@@ -491,6 +490,26 @@ module RestClient
 
     private
 
+    # Parse the @url string into a URI object using #parse_url and save it as
+    # @uri. Also save any basic auth user or password as @user and @password.
+    # If no auth info was passed, check for credentials in a Netrc file.
+    #
+    # @param [String] url A URL string.
+    #
+    # @return [URI]
+    #
+    # @raise URI::InvalidURIError on invalid URIs
+    #
+    def parse_url_with_auth!(url)
+      uri = parse_url(url)
+      @user = CGI.unescape(uri.user) if uri.user
+      @password = CGI.unescape(uri.password) if uri.password
+      if !@user && !@password
+        @user, @password = Netrc.read[uri.hostname]
+      end
+      @uri = uri
+    end
+
     def print_verify_callback_warnings
       warned = false
       if RestClient::Platform.mac_mri?
@@ -577,7 +596,6 @@ module RestClient
 
       log_request
 
-
       net.start do |http|
         established_connection = true
 

spec/unit/request_spec.rb

@@ -100,34 +100,43 @@ describe RestClient::Request, :include_helpers do
       URI.should_receive(:parse).with('HTTPS://example.com/resource')
       @request.parse_url('HTTPS://example.com/resource')
     end
+
+    it 'raises with invalid URI' do
+      lambda {
+        @request.parse_url('http://a@b:c')
+      }.should raise_error(URI::InvalidURIError, /\Athe scheme http/)
+      lambda {
+        @request.parse_url('http://')
+      }.should raise_error(URI::InvalidURIError, /\Abad URI/)
+    end
   end
 
   describe "user - password" do
     it "extracts the username and password when parsing http://user:password@example.com/" do
       URI.stub(:parse).and_return(double('uri', :user => 'joe', :password => 'pass1'))
-      @request.parse_url_with_auth('http://joe:pass1@example.com/resource')
+      @request.send(:parse_url_with_auth!, 'http://joe:pass1@example.com/resource')
       @request.user.should eq 'joe'
       @request.password.should eq 'pass1'
     end
 
     it "extracts with escaping the username and password when parsing http://user:password@example.com/" do
       URI.stub(:parse).and_return(double('uri', :user => 'joe%20', :password => 'pass1'))
-      @request.parse_url_with_auth('http://joe%20:pass1@example.com/resource')
+      @request.send(:parse_url_with_auth!, 'http://joe%20:pass1@example.com/resource')
       @request.user.should eq 'joe '
       @request.password.should eq 'pass1'
     end
 
     it "doesn't overwrite user and password (which may have already been set by the Resource constructor) if there is no user/password in the url" do
-      URI.stub(:parse).and_return(double('uri', :user => nil, :password => nil))
+      URI.stub(:parse).and_return(double('uri', user: nil, password: nil, hostname: 'example.com'))
       @request = RestClient::Request.new(:method => 'get', :url => 'example.com', :user => 'beth', :password => 'pass2')
-      @request.parse_url_with_auth('http://example.com/resource')
+      @request.send(:parse_url_with_auth!, 'http://example.com/resource')
       @request.user.should eq 'beth'
       @request.password.should eq 'pass2'
     end
   end
 
   it "correctly formats cookies provided to the constructor" do
-    URI.stub(:parse).and_return(double('uri', :user => nil, :password => nil))
+    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'})
@@ -171,7 +180,7 @@ describe RestClient::Request, :include_helpers do
   it "uses netrc credentials" do
     URI.stub(:parse).and_return(double('uri', :user => nil, :password => nil, :hostname => 'example.com'))
     Netrc.stub(:read).and_return('example.com' => ['a', 'b'])
-    @request.parse_url_with_auth('http://example.com/resource')
+    @request.send(:parse_url_with_auth!, 'http://example.com/resource')
     @request.user.should eq 'a'
     @request.password.should eq 'b'
   end
@@ -179,7 +188,7 @@ describe RestClient::Request, :include_helpers do
   it "uses credentials in the url in preference to netrc" do
     URI.stub(:parse).and_return(double('uri', :user => 'joe%20', :password => 'pass1', :hostname => 'example.com'))
     Netrc.stub(:read).and_return('example.com' => ['a', 'b'])
-    @request.parse_url_with_auth('http://joe%20:pass1@example.com/resource')
+    @request.send(:parse_url_with_auth!, 'http://joe%20:pass1@example.com/resource')
     @request.user.should eq 'joe '
     @request.password.should eq 'pass1'
   end
@@ -254,11 +263,10 @@ describe RestClient::Request, :include_helpers do
   end
 
   it "executes by constructing the Net::HTTP object, headers, and payload and calling transmit" do
-    @request.should_receive(:parse_url_with_auth).with('http://some/resource').and_return(@uri)
     klass = double("net:http class")
     @request.should_receive(:net_http_request_class).with(:put).and_return(klass)
     klass.should_receive(:new).and_return('result')
-    @request.should_receive(:transmit).with(@uri, 'result', kind_of(RestClient::Payload::Base))
+    @request.should_receive(:transmit).with(@request.uri, 'result', kind_of(RestClient::Payload::Base))
     @request.execute
   end
 
@@ -565,12 +573,6 @@ describe RestClient::Request, :include_helpers do
       RestClient::Request.new(:method => :get, :url => 'http://user:password@url', :headers => {:user_agent => 'rest-client'}).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'}).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
@@ -1121,4 +1123,15 @@ describe RestClient::Request, :include_helpers do
       @request.execute
     end
   end
+
+  describe 'constructor' do
+    it 'should reject invalid URIs' do
+      lambda {
+        RestClient::Request.new(method: :get, url: 'http://')
+      }.should raise_error(URI::InvalidURIError, /\Abad URI/)
+      lambda {
+        RestClient::Request.new(method: :get, url: 'http://a@b:c')
+      }.should raise_error(URI::InvalidURIError, /\Athe scheme http/)
+    end
+  end
 end