From a983ee7d60cbb58bc972826147f309e0cd38f970 Mon Sep 17 00:00:00 2001 From: "Daniel (dB.) Doubrovkine" Date: Sat, 7 Nov 2026 07:40:13 -0500 Subject: [PATCH] Fix relative redirects, bound redirect chains, and support plain HTTP Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + Gemfile | 2 + README.md | 12 ++--- lib/ruby-link-checker/checker.rb | 2 +- lib/ruby-link-checker/config.rb | 8 +++ lib/ruby-link-checker/errors.rb | 1 + .../errors/redirect_limit_error.rb | 15 ++++++ lib/ruby-link-checker/net/http/checker.rb | 2 +- lib/ruby-link-checker/tasks.rb | 10 ++-- .../async/http/checker_spec.rb | 3 +- spec/ruby-link-checker/checker_spec.rb | 4 ++ spec/ruby-link-checker/config_spec.rb | 6 +++ .../net/http/checker_spec.rb | 15 ++++++ .../typhoeus/hydra/checker_spec.rb | 2 +- spec/support/link_checker.rb | 53 ++++++++++++++++++- 15 files changed, 122 insertions(+), 14 deletions(-) create mode 100644 lib/ruby-link-checker/errors/redirect_limit_error.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f69ec1f..54d1562 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/Gemfile b/Gemfile index a415ca3..a6d6e34 100644 --- a/Gemfile +++ b/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' diff --git a/README.md b/README.md index 9947022..4d29557 100644 --- a/README.md +++ b/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 diff --git a/lib/ruby-link-checker/checker.rb b/lib/ruby-link-checker/checker.rb index 4797892..44e4327 100644 --- a/lib/ruby-link-checker/checker.rb +++ b/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| diff --git a/lib/ruby-link-checker/config.rb b/lib/ruby-link-checker/config.rb index 4bb7d9a..05d36f3 100644 --- a/lib/ruby-link-checker/config.rb +++ b/lib/ruby-link-checker/config.rb @@ -10,6 +10,7 @@ module Config headers logger retries + max_redirects ].freeze attr_accessor(*Config::ATTRIBUTES) @@ -20,6 +21,7 @@ def reset self.headers = {} self.logger = nil self.retries = 0 + self.max_redirects = 20 end def retries=(value) @@ -27,6 +29,12 @@ def retries=(value) @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 diff --git a/lib/ruby-link-checker/errors.rb b/lib/ruby-link-checker/errors.rb index 05768ea..1a5ea2c 100644 --- a/lib/ruby-link-checker/errors.rb +++ b/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' diff --git a/lib/ruby-link-checker/errors/redirect_limit_error.rb b/lib/ruby-link-checker/errors/redirect_limit_error.rb new file mode 100644 index 0000000..094a3d0 --- /dev/null +++ b/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 diff --git a/lib/ruby-link-checker/net/http/checker.rb b/lib/ruby-link-checker/net/http/checker.rb index 6f16ebe..81aaab5 100644 --- a/lib/ruby-link-checker/net/http/checker.rb +++ b/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) diff --git a/lib/ruby-link-checker/tasks.rb b/lib/ruby-link-checker/tasks.rb index dd9afc0..3f95987 100644 --- a/lib/ruby-link-checker/tasks.rb +++ b/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 diff --git a/spec/ruby-link-checker/async/http/checker_spec.rb b/spec/ruby-link-checker/async/http/checker_spec.rb index a0da95e..2ce8da7 100644 --- a/spec/ruby-link-checker/async/http/checker_spec.rb +++ b/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 diff --git a/spec/ruby-link-checker/checker_spec.rb b/spec/ruby-link-checker/checker_spec.rb index 2ed1471..4a88f9e 100644 --- a/spec/ruby-link-checker/checker_spec.rb +++ b/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 \ No newline at end of file diff --git a/spec/ruby-link-checker/config_spec.rb b/spec/ruby-link-checker/config_spec.rb index 6699627..4b0050a 100644 --- a/spec/ruby-link-checker/config_spec.rb +++ b/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 diff --git a/spec/ruby-link-checker/net/http/checker_spec.rb b/spec/ruby-link-checker/net/http/checker_spec.rb index cb2faeb..7a5b881 100644 --- a/spec/ruby-link-checker/net/http/checker_spec.rb +++ b/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 diff --git a/spec/ruby-link-checker/typhoeus/hydra/checker_spec.rb b/spec/ruby-link-checker/typhoeus/hydra/checker_spec.rb index 025a35b..8e823d1 100644 --- a/spec/ruby-link-checker/typhoeus/hydra/checker_spec.rb +++ b/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 diff --git a/spec/support/link_checker.rb b/spec/support/link_checker.rb index 783f545..a1a8782 100644 --- a/spec/support/link_checker.rb +++ b/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)