Refactor redirector logic

lib/http/chainable.rb

@@ -106,7 +106,7 @@ def stream
     # @param opts
     # @return [HTTP::Client]
     # @see Redirector#initialize
-    def follow(opts = true)
+    def follow(opts = {})
       branch default_options.with_follow opts
     end
 

lib/http/client.rb

@@ -39,13 +39,11 @@ def request(verb, uri, opts = {})
       req = HTTP::Request.new(verb, uri, headers, proxy, body)
       res = perform req, opts
 
-      if opts.follow
-        res = Redirector.new(opts.follow).perform req, res do |request|
-          perform request, opts
-        end
-      end
+      return res unless opts.follow
 
-      res
+      Redirector.new(opts.follow).perform req, res do |request|
+        perform request, opts
+      end
     end
 
     # Perform a single (no follow) HTTP request

lib/http/connection.rb

@@ -131,15 +131,14 @@ def expired?
     def start_tls(req, options)
       return unless req.uri.is_a?(URI::HTTPS) && !req.using_proxy?
 
-      ssl_class   = options[:ssl_socket_class]
       ssl_context = options[:ssl_context]
 
       unless ssl_context
         ssl_context = OpenSSL::SSL::SSLContext.new
-        ssl_context.set_params options[:ssl] || {}
+        ssl_context.set_params(options[:ssl] || {})
       end
 
-      @socket.start_tls(req.uri.host, ssl_class, ssl_context)
+      @socket.start_tls(req.uri.host, options[:ssl_socket_class], ssl_context)
     end
 
     # Resets expiration of persistent connection.

lib/http/options.rb

@@ -72,6 +72,15 @@ def initialize(options = {})
       def_option method_name
     end
 
+    def follow=(value)
+      @follow = case
+                when !value                    then nil
+                when true == value             then {}
+                when value.respond_to?(:fetch) then value
+                else argument_error! "Unsupported follow options: #{value}"
+                end
+    end
+
     def_option :cache do |cache_or_cache_options|
       if cache_or_cache_options.respond_to? :perform
         cache_or_cache_options

lib/http/redirector.rb

@@ -1,3 +1,5 @@
+require "set"
+
 module HTTP
   class Redirector
     # Notifies that we reached max allowed redirect hops
@@ -7,60 +9,88 @@ class TooManyRedirectsError < ResponseError; end
     class EndlessRedirectError < TooManyRedirectsError; end
 
     # HTTP status codes which indicate redirects
-    REDIRECT_CODES = [300, 301, 302, 303, 307, 308].freeze
+    REDIRECT_CODES = [300, 301, 302, 303, 307, 308].to_set.freeze
 
-    # :nodoc:
-    def initialize(options = nil)
-      options   = {:max_hops => 5} unless options.respond_to?(:fetch)
-      @max_hops = options.fetch(:max_hops, 5)
-      @max_hops = false if @max_hops && 1 > @max_hops.to_i
-    end
+    # Codes which which should raise StateError in strict mode if original
+    # request was any of {UNSAFE_VERBS}
+    STRICT_SENSITIVE_CODES = [300, 301, 302].to_set.freeze
 
-    # Follows redirects until non-redirect response found
-    def perform(request, response, &block)
-      reset(request, response)
-      follow(&block)
-    end
+    # Insecure http verbs, which should trigger StateError in strict mode
+    # upon {STRICT_SENSITIVE_CODES}
+    UNSAFE_VERBS = [:put, :delete, :post].to_set.freeze
 
-    private
+    # Verbs which will remain unchanged upon See Other response.
+    SEE_OTHER_ALLOWED_VERBS = [:get, :head].to_set.freeze
+
+    # @!attribute [r] strict
+    #   Returns redirector policy.
+    #   @return [Boolean]
+    attr_reader :strict
 
-    # Reset redirector state
-    def reset(request, response)
-      @request, @response = request, response
-      @visited = []
+    # @!attribute [r] max_hops
+    #   Returns maximum allowed hops.
+    #   @return [Fixnum]
+    attr_reader :max_hops
+
+    # @param [#to_h] opts
+    # @option opts [Boolean] :strict (true) redirector hops policy
+    # @option opts [#to_i] :max_hops (5) maximum allowed amount of hops
+    def initialize(options = {})
+      options   = options.to_h
+
+      @strict   = options.fetch(:strict, true)
+      @max_hops = options.fetch(:max_hops, 5).to_i
     end
 
-    # Follow redirects
-    def follow
-      while REDIRECT_CODES.include?(@response.code)
-        @visited << @request.uri.to_s
+    # Follows redirects until non-redirect response found
+    def perform(request, response)
+      @request  = request
+      @response = response
+      @visited  = []
+
+      while REDIRECT_CODES.include? @response.status.code
+        @visited << "#{@request.verb} #{@request.uri}"
 
         fail TooManyRedirectsError if too_many_hops?
         fail EndlessRedirectError  if endless_loop?
 
-        uri = @response.headers["Location"]
-        fail StateError, "no Location header in redirect" unless uri
-
-        if 303 == @response.code
-          @request = @request.redirect uri, :get
-        else
-          @request = @request.redirect uri
-        end
-
+        @request  = redirect_to @response.headers["Location"]
         @response = yield @request
       end
 
       @response
     end
 
+    private
+
     # Check if we reached max amount of redirect hops
+    # @return [Boolean]
     def too_many_hops?
-      @max_hops < @visited.count if @max_hops
+      1 <= @max_hops && @max_hops < @visited.count
     end
 
     # Check if we got into an endless loop
+    # @return [Boolean]
     def endless_loop?
-      2 < @visited.count(@visited.last)
+      2 <= @visited.count(@visited.last)
+    end
+
+    # Redirect policy for follow
+    # @return [Request]
+    def redirect_to(uri)
+      fail StateError, "no Location header in redirect" unless uri
+
+      verb = @request.verb
+      code = @response.status.code
+
+      if UNSAFE_VERBS.include?(verb) && STRICT_SENSITIVE_CODES.include?(code)
+        fail StateError, "can't follow #{@response.status} redirect" if @strict
+        verb = :get
+      end
+
+      verb = :get if !SEE_OTHER_ALLOWED_VERBS.include?(verb) && 303 == code
+
+      @request.redirect(uri, verb)
     end
   end
 end

spec/lib/http/client_spec.rb

@@ -60,7 +60,7 @@ def simple_response(body, status = 200)
     end
 
     it "fails if max amount of hops reached" do
-      client = StubbedClient.new(:follow => 5).stub(
+      client = StubbedClient.new(:follow => {:max_hops => 5}).stub(
         "http://example.com/"  => redirect_response("/1"),
         "http://example.com/1" => redirect_response("/2"),
         "http://example.com/2" => redirect_response("/3"),