Merge pull request #752 from riccardoporreca/feature/750-fail-on-prot…

…ocol-relative-urls

Fail on protocol-relative urls

lib/html_proofer.rb

@@ -20,37 +20,39 @@
 end
 
 module HTMLProofer
-  def self.check_file(file, options = {})
-    raise ArgumentError unless file.is_a?(String)
-    raise ArgumentError, "#{file} does not exist" unless File.exist?(file)
+  class << self
+    def check_file(file, options = {})
+      raise ArgumentError unless file.is_a?(String)
+      raise ArgumentError, "#{file} does not exist" unless File.exist?(file)
 
-    options[:type] = :file
-    HTMLProofer::Runner.new(file, options)
-  end
+      options[:type] = :file
+      HTMLProofer::Runner.new(file, options)
+    end
 
-  def self.check_directory(directory, options = {})
-    raise ArgumentError unless directory.is_a?(String)
-    raise ArgumentError, "#{directory} does not exist" unless Dir.exist?(directory)
+    def check_directory(directory, options = {})
+      raise ArgumentError unless directory.is_a?(String)
+      raise ArgumentError, "#{directory} does not exist" unless Dir.exist?(directory)
 
-    options[:type] = :directory
-    HTMLProofer::Runner.new([directory], options)
-  end
+      options[:type] = :directory
+      HTMLProofer::Runner.new([directory], options)
+    end
 
-  def self.check_directories(directories, options = {})
-    raise ArgumentError unless directories.is_a?(Array)
+    def check_directories(directories, options = {})
+      raise ArgumentError unless directories.is_a?(Array)
 
-    options[:type] = :directory
-    directories.each do |directory|
-      raise ArgumentError, "#{directory} does not exist" unless Dir.exist?(directory)
+      options[:type] = :directory
+      directories.each do |directory|
+        raise ArgumentError, "#{directory} does not exist" unless Dir.exist?(directory)
+      end
+      HTMLProofer::Runner.new(directories, options)
     end
-    HTMLProofer::Runner.new(directories, options)
-  end
 
-  def self.check_links(links, options = {})
-    raise ArgumentError unless links.is_a?(Array)
+    def check_links(links, options = {})
+      raise ArgumentError unless links.is_a?(Array)
 
-    options[:type] = :links
-    HTMLProofer::Runner.new(links, options)
+      options[:type] = :links
+      HTMLProofer::Runner.new(links, options)
+    end
   end
 end
 

lib/html_proofer/attribute/url.rb

@@ -20,12 +20,13 @@ def initialize(runner, link_attribute, base_url: nil, extract_size: false)
 
           swap_urls!
           clean_url!
-
-          # convert "//" links to "https://"
-          @url.start_with?("//") ? @url = "https:#{@url}" : @url
         end
       end
 
+      def protocol_relative?
+        url.start_with?("//")
+      end
+
       def to_s
         @url
       end

lib/html_proofer/check.rb

@@ -29,28 +29,10 @@ def add_failure(description, line: nil, status: nil, content: nil)
         content: content)
     end
 
-    def self.subchecks(runner_options)
-      # grab all known checks
-      checks = ObjectSpace.each_object(Class).select do |klass|
-        klass < self
-      end
-
-      # remove any checks not explicitly included
-      checks.each_with_object([]) do |check, arr|
-        next unless runner_options[:checks].include?(check.short_name)
-
-        arr << check
-      end
-    end
-
     def short_name
       self.class.name.split("::").last
     end
 
-    def self.short_name
-      name.split("::").last
-    end
-
     def add_to_internal_urls(url, line)
       url_string = url.raw_attribute
 
@@ -74,6 +56,26 @@ def add_to_external_urls(url, line)
       @external_urls[url_string] << { filename: @runner.current_filename, line: line }
     end
 
+    class << self
+      def subchecks(runner_options)
+        # grab all known checks
+        checks = ObjectSpace.each_object(Class).select do |klass|
+          klass < self
+        end
+
+        # remove any checks not explicitly included
+        checks.each_with_object([]) do |check, arr|
+          next unless runner_options[:checks].include?(check.short_name)
+
+          arr << check
+        end
+      end
+
+      def short_name
+        name.split("::").last
+      end
+    end
+
     private def base_url
       return @base_url if defined?(@base_url)
 

lib/html_proofer/check/favicon.rb

@@ -16,7 +16,10 @@ def run
         return if immediate_redirect?
 
         if found
-          if @favicon.url.remote?
+          if @favicon.url.protocol_relative?
+            add_failure("favicon link #{@favicon.url} is a protocol-relative URL, use explicit https:// instead",
+              line: @favicon.line, content: @favicon.content)
+          elsif @favicon.url.remote?
             add_to_external_urls(@favicon.url, @favicon.line)
           elsif !@favicon.url.exists?
             add_failure("internal favicon #{@favicon.url.raw_attribute} does not exist", line: @favicon.line,

lib/html_proofer/check/images.rb

@@ -18,6 +18,9 @@ def run
           # does the image exist?
           if missing_src?
             add_failure("image has no src or srcset attribute", line: @img.line, content: @img.content)
+          elsif @img.url.protocol_relative?
+            add_failure("image link #{@img.url} is a protocol-relative URL, use explicit https:// instead",
+              line: @img.line, content: @img.content)
           elsif @img.url.remote?
             add_to_external_urls(@img.url, @img.line)
           elsif !@img.url.exists? && !@img.multiple_srcsets? && !@img.multiple_sizes?
@@ -27,7 +30,10 @@ def run
             @img.srcsets_wo_sizes.each do |srcset|
               srcset_url = HTMLProofer::Attribute::Url.new(@runner, srcset, base_url: @img.base_url, extract_size: true)
 
-              if srcset_url.remote?
+              if srcset_url.protocol_relative?
+                add_failure("image link #{srcset_url.url} is a protocol-relative URL, use explicit https:// instead",
+                  line: @img.line, content: @img.content)
+              elsif srcset_url.remote?
                 add_to_external_urls(srcset_url.url, @img.line)
               elsif !srcset_url.exists?
                 add_failure("internal image #{srcset} does not exist", line: @img.line, content: @img.content)

lib/html_proofer/check/links.rb

@@ -28,6 +28,12 @@ def run
             next
           end
 
+          if @link.url.protocol_relative?
+            add_failure("#{@link.url} is a protocol-relative URL, use explicit https:// instead",
+              line: @link.line, content: @link.content)
+            next
+          end
+
           check_schemes
 
           # intentionally down here because we still want valid? & missing_href? to execute

lib/html_proofer/check/open_graph.rb

@@ -16,6 +16,9 @@ def run
             add_failure("open graph content attribute is empty", line: @open_graph.line, content: @open_graph.content)
           elsif !@open_graph.url.valid?
             add_failure("#{@open_graph.src} is an invalid URL", line: @open_graph.line)
+          elsif @open_graph.url.protocol_relative?
+            add_failure("open graph link #{@open_graph.url} is a protocol-relative URL, use explicit https:// instead",
+              line: @open_graph.line, content: @open_graph.content)
           elsif @open_graph.url.remote?
             add_to_external_urls(@open_graph.url, @open_graph.line)
           else

lib/html_proofer/check/scripts.rb

@@ -13,8 +13,11 @@ def run
           # does the script exist?
           if missing_src?
             add_failure("script is empty and has no src attribute", line: @script.line, content: @script.content)
+          elsif @script.url.protocol_relative?
+            add_failure("script link #{@script.url} is a protocol-relative URL, use explicit https:// instead",
+              line: @script.line, content: @script.content)
           elsif @script.url.remote?
-            add_to_external_urls(@script.src, @script.line)
+            add_to_external_urls(@script.url, @script.line)
             check_sri if @runner.check_sri?
           elsif !@script.url.exists?
             add_failure("internal script reference #{@script.src} does not exist", line: @script.line,

lib/html_proofer/configuration.rb

@@ -47,42 +47,44 @@ module Configuration
 
     CACHE_DEFAULTS = {}.freeze
 
-    def self.generate_defaults(opts)
-      options = PROOFER_DEFAULTS.merge(opts)
+    class << self
+      def generate_defaults(opts)
+        options = PROOFER_DEFAULTS.merge(opts)
 
-      options[:typhoeus] = HTMLProofer::Configuration::TYPHOEUS_DEFAULTS.merge(opts[:typhoeus] || {})
-      options[:hydra] = HTMLProofer::Configuration::HYDRA_DEFAULTS.merge(opts[:hydra] || {})
+        options[:typhoeus] = HTMLProofer::Configuration::TYPHOEUS_DEFAULTS.merge(opts[:typhoeus] || {})
+        options[:hydra] = HTMLProofer::Configuration::HYDRA_DEFAULTS.merge(opts[:hydra] || {})
 
-      options[:parallel] = HTMLProofer::Configuration::PARALLEL_DEFAULTS.merge(opts[:parallel] || {})
-      options[:cache] = HTMLProofer::Configuration::CACHE_DEFAULTS.merge(opts[:cache] || {})
+        options[:parallel] = HTMLProofer::Configuration::PARALLEL_DEFAULTS.merge(opts[:parallel] || {})
+        options[:cache] = HTMLProofer::Configuration::CACHE_DEFAULTS.merge(opts[:cache] || {})
 
-      options.delete(:src)
+        options.delete(:src)
 
-      options
-    end
+        options
+      end
 
-    def self.to_regex?(item)
-      if item.start_with?("/") && item.end_with?("/")
-        Regexp.new(item[1...-1])
-      else
-        item
+      def to_regex?(item)
+        if item.start_with?("/") && item.end_with?("/")
+          Regexp.new(item[1...-1])
+        else
+          item
+        end
       end
-    end
 
-    def self.parse_json_option(option_name, config, symbolize_names: true)
-      raise ArgumentError, "Must provide an option name in string format." unless option_name.is_a?(String)
-      raise ArgumentError, "Must provide an option name in string format." if option_name.strip.empty?
+      def parse_json_option(option_name, config, symbolize_names: true)
+        raise ArgumentError, "Must provide an option name in string format." unless option_name.is_a?(String)
+        raise ArgumentError, "Must provide an option name in string format." if option_name.strip.empty?
 
-      return {} if config.nil?
+        return {} if config.nil?
 
-      raise ArgumentError, "Must provide a JSON configuration in string format." unless config.is_a?(String)
+        raise ArgumentError, "Must provide a JSON configuration in string format." unless config.is_a?(String)
 
-      return {} if config.strip.empty?
+        return {} if config.strip.empty?
 
-      begin
-        JSON.parse(config, { symbolize_names: symbolize_names })
-      rescue StandardError
-        raise ArgumentError, "Option '#{option_name} did not contain valid JSON."
+        begin
+          JSON.parse(config, { symbolize_names: symbolize_names })
+        rescue StandardError
+          raise ArgumentError, "Option '#{option_name} did not contain valid JSON."
+        end
       end
     end
   end

spec/html-proofer/attribute/url_spec.rb

@@ -20,4 +20,31 @@
       expect(url.ignore?).to(eq(true))
     end
   end
+
+  describe "#protocol_relative" do
+    it "works for protocol relative" do
+      url = HTMLProofer::Attribute::Url.new(@runner, "//assets/main.js")
+      expect(url.protocol_relative?).to(eq(true))
+    end
+
+    it "works for https://" do
+      url = HTMLProofer::Attribute::Url.new(@runner, "https://assets/main.js")
+      expect(url.protocol_relative?).to(eq(false))
+    end
+
+    it "works for http://" do
+      url = HTMLProofer::Attribute::Url.new(@runner, "http://assets/main.js")
+      expect(url.protocol_relative?).to(eq(false))
+    end
+
+    it "works for relative internal link" do
+      url = HTMLProofer::Attribute::Url.new(@runner, "assets/main.js")
+      expect(url.protocol_relative?).to(eq(false))
+    end
+
+    it "works for absolute internal link" do
+      url = HTMLProofer::Attribute::Url.new(@runner, "/assets/main.js")
+      expect(url.protocol_relative?).to(eq(false))
+    end
+  end
 end

spec/html-proofer/cli_reporter_spec.rb

@@ -45,21 +45,21 @@
 
           internal image ./Screen Shot 2012-08-09 at 7.51.18 AM.png does not exist
 
-        For the Links check, the following failures were found:
+        * At spec/html-proofer/fixtures/sorting/kitchen_sinkish.html:14:
 
-        * At spec/html-proofer/fixtures/sorting/kitchen_sinkish.html:8:
+          image link //upload.wikimedia.org/wikipedia/en/thumb/not_here.png is a protocol-relative URL, use explicit https:// instead
 
-          tel: contains no phone number
+        * At spec/html-proofer/fixtures/sorting/kitchen_sinkish.html:19:
 
-        For the Links > External check, the following failures were found:
+          image link //upload.wikimedia.org/wikipedia/en/thumb/fooooof.png is a protocol-relative URL, use explicit https:// instead
 
-        * At spec/html-proofer/fixtures/sorting/kitchen_sinkish.html:14:
+        For the Links check, the following failures were found:
 
-          External link https://upload.wikimedia.org/wikipedia/en/thumb/not_here.png failed (status code 404)
+        * At spec/html-proofer/fixtures/sorting/kitchen_sinkish.html:8:
 
-        * At spec/html-proofer/fixtures/sorting/kitchen_sinkish.html:19:
+          tel: contains no phone number
 
-          External link https://upload.wikimedia.org/wikipedia/en/thumb/fooooof.png failed (status code 404)
+        For the Links > External check, the following failures were found:
 
         * At spec/html-proofer/fixtures/sorting/kitchen_sinkish.html:26:
 

spec/html-proofer/favicon_spec.rb

@@ -20,6 +20,12 @@
     expect(proofer.failed_checks.last.description).to(match(/(no favicon provided)/))
   end
 
+  it "fails for favicon missing the protocol" do
+    missing_protocol = File.join(FIXTURES_DIR, "favicon", "protocol_relative_favicon.html")
+    proofer = run_proofer(missing_protocol, :file)
+    expect(proofer.failed_checks.first.description).to(match(/protocol-relative URL/))
+  end
+
   it "fails for broken internal favicon" do
     broken = File.join(FIXTURES_DIR, "favicon", "internal_favicon_broken.html")
     proofer = run_proofer(broken, :file, checks: ["Favicon"])

spec/html-proofer/fixtures/favicon/protocol_relative_favicon.html

@@ -0,0 +1,6 @@
+<html>
+  <head>
+    <link rel="icon" href="//www.github.com/asdadaskdalsdk.png" />
+  </head>
+  <body></body>
+</html>

spec/html-proofer/fixtures/opengraph/image-missing-protocol.html

@@ -0,0 +1 @@
+<meta property="og:image" content="//github.com/favicon.ico" />

spec/html-proofer/fixtures/opengraph/url-missing-protocol.html

@@ -0,0 +1 @@
+<meta property="og:url" content="//www.github.com/">

spec/html-proofer/fixtures/scripts/script_missing_protocol.html

@@ -0,0 +1,9 @@
+<html>
+
+<body>
+
+  <script src="//www.asdo3IRJ395295jsingrkrg4.com/asdo3IRJ.js"></script>
+
+</body>
+
+</html>

spec/html-proofer/images_spec.rb

@@ -122,16 +122,10 @@
     expect(proofer.failed_checks).to(eq([]))
   end
 
-  it "works for valid images missing the protocol" do
+  it "fails for images missing the protocol" do
     missing_protocol_link = File.join(FIXTURES_DIR, "images", "image_missing_protocol_valid.html")
     proofer = run_proofer(missing_protocol_link, :file)
-    expect(proofer.failed_checks).to(eq([]))
-  end
-
-  it "fails for invalid images missing the protocol" do
-    missing_protocol_link = File.join(FIXTURES_DIR, "images", "image_missing_protocol_invalid.html")
-    proofer = run_proofer(missing_protocol_link, :file)
-    expect(proofer.failed_checks.first.description).to(match(/failed/))
+    expect(proofer.failed_checks.first.description).to(match(/protocol-relative URL/))
   end
 
   it "properly checks relative links" do