Fix chained redirects and plain HTTP checks

CHANGELOG.md

@@ -1,5 +1,6 @@
 ### 0.5.0 (Next)
 
+* Fixed chained relative redirects, bounded redirect following with `max_redirects`, enabled plain HTTP in the Net::HTTP checker, and corrected retry documentation - [@dblock](https://github.com/dblock), [@Copilot](https://github.com/Copilot).
 * [#8](https://github.com/dblock/ruby-link-checker/issues/8): Added `protocol` configuration option to `LinkChecker::Async::HTTP` to force HTTP/1 or HTTP/2 - [@dblock](https://github.com/dblock), [@Copilot](https://github.com/Copilot).
 * Your contribution here.
 

Gemfile

@@ -11,6 +11,8 @@ group :development, :test do
   gem 'danger-changelog', '0.8.0'
   gem 'danger-pr-comment'
   gem 'danger-toc', '0.2.0'
+  # Temporary until Danger's git 6 support lands: https://github.com/danger/danger/pull/1561
+  gem 'git', '< 6', require: false
   gem 'rspec', '~> 3.0'
   gem 'rubocop', '~> 1.0'
   gem 'rubocop-rake'

README.md

@@ -19,7 +19,7 @@ A fast Ruby link checker with support for multiple HTTP libraries. Does not pars
     - [LinkChecker::Net::HTTP](#linkcheckernethttp)
   - [Options](#options)
     - [Retries](#retries)
-    - [Retry After](#retry-after)
+    - [Redirects](#redirects)
     - [Results](#results)
     - [Methods](#methods)
     - [Logger](#logger)
@@ -218,18 +218,18 @@ end
 
 #### Retries
 
-By default link checkers do not retry. You can set a number of times to retry all errors and failures with `retries`.
+By default link checkers do not retry. Set `retries` to retry errors and failures. The retry budget is shared across methods: with `retries: 2` and the default methods, repeated failures produce `HEAD`, `HEAD`, `HEAD`, then `GET`.
 
 ```ruby
-checker = LinkChecker::Net::HTTP::Checker.new(retry: 1)
+checker = LinkChecker::Net::HTTP::Checker.new(retries: 1)
 ```
 
-#### Retry After
+#### Redirects
 
-If retry is enabled, link checkers can wait for a number of seconds before retrying.
+Checkers follow up to 20 redirects per attempt and detect redirect loops. Relative redirects are resolved against the current response URL. Set `max_redirects` to change the limit, or use `0` to disable redirects.
 
 ```ruby
-checker = LinkChecker::Net::HTTP::Checker.new(retry_after: 1) # wait for 1 second before retrying
+checker = LinkChecker::Typhoeus::Hydra::Checker.new(max_redirects: 10)
 ```
 
 #### Results

lib/ruby-link-checker/checker.rb

@@ -6,7 +6,7 @@ class Checker
     include LinkChecker::Callbacks
 
     attr_reader :results
-    attr_accessor(*Config::ATTRIBUTES)
+    attr_accessor(*(Config::ATTRIBUTES - %i[max_redirects]))
 
     def initialize(options = {})
       LinkChecker::Config::ATTRIBUTES.each do |key|

lib/ruby-link-checker/config.rb

@@ -10,6 +10,7 @@ module Config
       headers
       logger
       retries
+      max_redirects
     ].freeze
 
     attr_accessor(*Config::ATTRIBUTES)
@@ -20,13 +21,20 @@ def reset
       self.headers = {}
       self.logger = nil
       self.retries = 0
+      self.max_redirects = 20
     end
 
     def retries=(value)
       raise ArgumentError, "Invalid number of retries: #{value}" unless value.is_a?(Integer) && value >= 0
 
       @retries = value
     end
+
+    def max_redirects=(value)
+      raise ArgumentError, "Invalid redirect limit: #{value}" unless value.is_a?(Integer) && value >= 0
+
+      @max_redirects = value
+    end
   end
 
   class << self

lib/ruby-link-checker/errors.rb

@@ -1,2 +1,3 @@
 require_relative 'errors/base_error'
 require_relative 'errors/redirect_loop_error'
+require_relative 'errors/redirect_limit_error'

lib/ruby-link-checker/errors/redirect_limit_error.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module LinkChecker
+  module Errors
+    class RedirectLimitError < BaseError
+      attr_reader :limit, :urls
+
+      def initialize(limit, urls)
+        @limit = limit
+        @urls = urls
+        super("Redirect limit of #{limit} exceeded: #{urls.join(' -> ')}.")
+      end
+    end
+  end
+end

lib/ruby-link-checker/net/http/checker.rb

@@ -3,7 +3,7 @@ module Net
     module HTTP
       class Task < ::LinkChecker::Task
         def run!
-          ::Net::HTTP.start(uri.host, uri.port, use_ssl: true) do |http|
+          ::Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == 'https') do |http|
             http.read_timeout = checker.read_timeout if checker.read_timeout
             http.open_timeout = checker.open_timeout if checker.open_timeout
             request = ::Net::HTTPGenericRequest.new(method, false, true, uri)

lib/ruby-link-checker/tasks.rb

@@ -29,6 +29,7 @@ def execute!
       elsif methods_left.any?
         @method = methods_left.shift
         @uri = URI(@uri) unless @uri.is_a?(URI)
+        @redirects = [@uri] if @result.nil?
         _queue_task(uri, method, original_uri || uri, options)
       elsif @result && result.error?
         error! @result
@@ -88,14 +89,17 @@ def _handle_result(result)
       result! result
       if result.redirect?
         redirect! result
-        redirected_to_uri = URI.join(uri, result.redirect_to)
+        redirected_to_uri = URI.join(result.result_uri, result.redirect_to)
         if redirects.include?(redirected_to_uri)
           raise LinkChecker::Errors::RedirectLoopError,
                 redirects.push(redirected_to_uri)
         end
+        if redirects.length - 1 >= checker.max_redirects
+          raise LinkChecker::Errors::RedirectLimitError.new(checker.max_redirects, redirects + [redirected_to_uri])
+        end
 
         redirects << redirected_to_uri
-        _queue_task(redirected_to_uri, result.method, uri, options)
+        _queue_task(redirected_to_uri, result.method, result.uri, options)
       elsif result.success?
         success! result
       else
@@ -104,7 +108,7 @@ def _handle_result(result)
       end
     rescue StandardError => e
       logger.error("#{self}##{__method__}") { e } rescue nil
-      error_result = ResultError.new(result.uri, result.method, result.result_uri, e, options)
+      error_result = ResultError.new(result.result_uri, result.method, result.uri, e, options)
       result! error_result
       error! error_result
     end

spec/ruby-link-checker/async/http/checker_spec.rb

@@ -21,9 +21,10 @@ def check(url, options = {})
       config.hook_into :webmock
       # async-http's WebMock adapter replays responses through a real HTTP/1
       # parser, which rejects a Transfer-Encoding header alongside a
-      # Content-Length or an empty body, unlike the other checkers' adapters.
+      # Content-Length or an empty body. Let the adapter set Content-Length.
       config.before_playback do |interaction|
         interaction.response.headers.delete('Transfer-Encoding')
+        interaction.response.headers.delete_if { |key, _value| key.downcase == 'content-length' }
       end
     end
   end

spec/ruby-link-checker/checker_spec.rb

@@ -6,5 +6,9 @@
     it 'requires at least one method' do
       expect { LinkChecker::Checker.new(methods: []) }.to raise_error ArgumentError, 'Missing methods.'
     end
+
+    it 'rejects an invalid redirect limit' do
+      expect { described_class.new(max_redirects: -1) }.to raise_error ArgumentError, 'Invalid redirect limit: -1'
+    end
   end
 end

spec/ruby-link-checker/config_spec.rb

@@ -19,6 +19,12 @@
       it 'requires a positive integer' do
         expect { LinkChecker.config.retries = -1 }.to raise_error ArgumentError, 'Invalid number of retries: -1'
       end
+
+      context 'max_redirects' do
+        it 'rejects negative limits' do
+          expect { LinkChecker.config.max_redirects = -1 }.to raise_error ArgumentError, 'Invalid redirect limit: -1'
+        end
+      end
     end
   end
 

spec/ruby-link-checker/net/http/checker_spec.rb

@@ -15,6 +15,21 @@
 
   it_behaves_like 'a link checker'
 
+  context 'with an HTTP URL' do
+    let(:checker) { described_class.new(methods: ['GET']) }
+
+    around do |example|
+      VCR.turned_off { example.run }
+    end
+
+    it 'does not enable TLS' do
+      stub_request(:get, 'http://www.example.org/').to_return(status: 200)
+      checker.check('http://www.example.org/')
+      expect(checker.results[:success].first.code).to eq(200)
+      expect(a_request(:get, 'http://www.example.org/')).to have_been_made.once
+    end
+  end
+
   describe '.config' do
     it 'returns the Config module' do
       expect(LinkChecker::Net::HTTP.config).to eq LinkChecker::Net::HTTP::Config

spec/ruby-link-checker/typhoeus/hydra/checker_spec.rb

@@ -15,7 +15,7 @@ def check(url, options = {})
   end
   before :all do
     VCR.configure do |config|
-      config.hook_into :typhoeus
+      config.hook_into :webmock
     end
   end
 

spec/support/link_checker.rb

@@ -1,4 +1,55 @@
 shared_context 'a link checker' do
+  context 'with redirects', :aggregate_failures do
+    let(:checker) { described_class.new(methods: ['GET'], max_redirects: 2) }
+    let(:url) { 'https://www.example.org/start' }
+    let(:result) { checker.results.values.flatten.first }
+
+    around do |example|
+      VCR.turned_off { example.run }
+    end
+
+    it 'resolves each relative redirect against the current URL' do
+      stub_request(:any, url).to_return(status: 302, headers: { 'Location' => '/nested/next' })
+      stub_request(:any, 'https://www.example.org/nested/next').to_return(status: 302, headers: { 'Location' => 'final' })
+      stub_request(:any, 'https://www.example.org/nested/final').to_return(status: 200)
+      checker.check(url, location: 'page.html')
+      checker.run if checker.respond_to?(:run)
+      expect(result.uri.to_s).to eq(url)
+      expect(result.result_uri.to_s).to eq('https://www.example.org/nested/final')
+      expect(result.options).to eq(location: 'page.html')
+    end
+
+    it 'stops redirect chains at the configured limit' do
+      stub_request(:any, url).to_return(status: 302, headers: { 'Location' => '/one' })
+      stub_request(:any, 'https://www.example.org/one').to_return(status: 302, headers: { 'Location' => '/two' })
+      stub_request(:any, 'https://www.example.org/two').to_return(status: 302, headers: { 'Location' => '/three' })
+      checker.check(url)
+      checker.run if checker.respond_to?(:run)
+      expect(result.error).to be_a(LinkChecker::Errors::RedirectLimitError)
+      expect(result.uri.to_s).to eq(url)
+      expect(result.result_uri.to_s).to eq('https://www.example.org/two')
+      expect(a_request(:any, 'https://www.example.org/three')).not_to have_been_made
+    end
+
+    it 'detects a redirect back to the original URL without requesting it again' do
+      stub_request(:any, url).to_return(status: 302, headers: { 'Location' => '/one' })
+      stub_request(:any, 'https://www.example.org/one').to_return(status: 302, headers: { 'Location' => url })
+      checker.check(url)
+      checker.run if checker.respond_to?(:run)
+      expect(checker.results[:error].first.error).to be_a(LinkChecker::Errors::RedirectLoopError)
+      expect(a_request(:any, url)).to have_been_made.once
+    end
+
+    it 'allows redirects to be disabled' do
+      checker.max_redirects = 0
+      stub_request(:any, url).to_return(status: 302, headers: { 'Location' => '/one' })
+      checker.check(url)
+      checker.run if checker.respond_to?(:run)
+      expect(checker.results[:error].first.error).to be_a(LinkChecker::Errors::RedirectLimitError)
+      expect(a_request(:any, 'https://www.example.org/one')).not_to have_been_made
+    end
+  end
+
   context 'default' do
     subject do
       described_class.new
@@ -240,7 +291,7 @@
               expect(result.error?).to be true
               expect(result.error).to be_a LinkChecker::Errors::RedirectLoopError
               expect(result.redirect?).to be false
-              expect(subject).to have_received(:called!).with(:redirect, anything).twice
+              expect(subject).to have_received(:called!).with(:redirect, anything).once
               expect(subject).to have_received(:called!).with(:error, result)
               expect(subject).not_to have_received(:called!).with(:failure, result)
               expect(subject).not_to have_received(:called!).with(:success, result)