Merge pull request #1637 from broder/mirror_audit

Updated resource auditing to detect invalid mirrors when using --online

Library/Homebrew/dev-cmd/audit.rb

@@ -174,30 +174,64 @@ class FormulaAuditor
     @specs = %w[stable devel head].map { |s| formula.send(s) }.compact
   end
 
-  def url_status_code(url, range: false)
-    # The system Curl is too old and unreliable with HTTPS homepages on
-    # Yosemite and below.
-    return "200" unless DevelopmentTools.curl_handles_most_https_homepages?
+  def self.check_http_content(url, user_agents: [:default])
+    return unless url.start_with? "http"
 
-    extra_args = [
-      "--connect-timeout", "15",
-      "--output", "/dev/null",
-      "--write-out", "%{http_code}"
-    ]
-    extra_args << "--range" << "0-0" if range
-    extra_args << url
-
-    status_code = nil
-    [:browser, :default].each do |user_agent|
-      args = curl_args(
-        extra_args: extra_args,
-        show_output: true,
-        user_agent: user_agent,
-      )
-      status_code = Open3.popen3(*args) { |_, stdout, _, _| stdout.read }
-      break if status_code.start_with? "20"
+    details = nil
+    user_agent = nil
+    user_agents.each do |ua|
+      details = http_content_headers_and_checksum(url, user_agent: ua)
+      user_agent = ua
+      break if details[:status].to_s.start_with?("2")
     end
-    status_code
+
+    return "The URL #{url} is not reachable" unless details[:status]
+    unless details[:status].start_with? "2"
+      return "The URL #{url} is not reachable (HTTP status code #{details[:status]})"
+    end
+
+    return unless url.start_with? "http:"
+
+    secure_url = url.sub "http", "https"
+    secure_details =
+      http_content_headers_and_checksum(secure_url, user_agent: user_agent)
+
+    if !details[:status].to_s.start_with?("2") ||
+       !secure_details[:status].to_s.start_with?("2")
+      return
+    end
+
+    etag_match = details[:etag] &&
+                 details[:etag] == secure_details[:etag]
+    content_length_match =
+      details[:content_length] &&
+      details[:content_length] == secure_details[:content_length]
+    file_match = details[:file_hash] == secure_details[:file_hash]
+
+    return if !etag_match && !content_length_match && !file_match
+    "The URL #{url} could use HTTPS rather than HTTP"
+  end
+
+  def self.http_content_headers_and_checksum(url, user_agent: :default)
+    args = curl_args(
+      extra_args: ["--connect-timeout", "15", "--include", url],
+      show_output: true,
+      user_agent: user_agent,
+    )
+    output = Open3.popen3(*args) { |_, stdout, _, _| stdout.read }
+
+    status_code = :unknown
+    while status_code == :unknown || status_code.to_s.start_with?("3")
+      headers, _, output = output.partition("\r\n\r\n")
+      status_code = headers[%r{HTTP\/.* (\d+)}, 1]
+    end
+
+    {
+      status: status_code,
+      etag: headers[%r{ETag: ([wW]\/)?"(([^"]|\\")*)"}, 2],
+      content_length: headers[/Content-Length: (\d+)/, 1],
+      file_hash: Digest::SHA256.digest(output),
+    }
   end
 
   def audit_style
@@ -619,9 +653,13 @@ class FormulaAuditor
 
     return unless @online
 
-    status_code = url_status_code(homepage)
-    return if status_code.start_with? "20"
-    problem "The homepage #{homepage} is not reachable (HTTP status code #{status_code})"
+    # The system Curl is too old and unreliable with HTTPS homepages on
+    # Yosemite and below.
+    return unless DevelopmentTools.curl_handles_most_https_homepages?
+    if http_content_problem = FormulaAuditor.check_http_content(homepage,
+                                             user_agents: [:browser, :default])
+      problem http_content_problem
+    end
   end
 
   def audit_bottle_spec
@@ -671,11 +709,11 @@ class FormulaAuditor
     %w[Stable Devel HEAD].each do |name|
       next unless spec = formula.send(name.downcase)
 
-      ra = ResourceAuditor.new(spec, online: @online).audit
+      ra = ResourceAuditor.new(spec, online: @online, strict: @strict).audit
       problems.concat ra.problems.map { |problem| "#{name}: #{problem}" }
 
       spec.resources.each_value do |resource|
-        ra = ResourceAuditor.new(resource, online: @online).audit
+        ra = ResourceAuditor.new(resource, online: @online, strict: @strict).audit
         problems.concat ra.problems.map { |problem|
           "#{name} resource #{resource.name.inspect}: #{problem}"
         }
@@ -1231,6 +1269,7 @@ class ResourceAuditor
     @using    = resource.using
     @specs    = resource.specs
     @online   = options[:online]
+    @strict   = options[:strict]
     @problems = []
   end
 
@@ -1490,38 +1529,26 @@ class ResourceAuditor
 
     return unless @online
     urls.each do |url|
-      check_insecure_mirror(url) if url.start_with? "http:"
+      next if !@strict && mirrors.include?(url)
+
+      strategy = DownloadStrategyDetector.detect(url, using)
+      if strategy <= CurlDownloadStrategy && !url.start_with?("file")
+        if http_content_problem = FormulaAuditor.check_http_content(url)
+          problem http_content_problem
+        end
+      elsif strategy <= GitDownloadStrategy
+        unless Utils.git_remote_exists url
+          problem "The URL #{url} is not a valid git URL"
+        end
+      elsif strategy <= SubversionDownloadStrategy
+        unless Utils.svn_remote_exists url
+          problem "The URL #{url} is not a valid svn URL"
+        end
+      end
     end
   end
 
-  def check_insecure_mirror(url)
-    details =  get_content_details(url)
-    secure_url = url.sub "http", "https"
-    secure_details = get_content_details(secure_url)
-
-    return if details[:status].nil? || secure_details[:status].nil? || !details[:status].start_with?("2") || !secure_details[:status].start_with?("2")
-
-    etag_match = details[:etag] && details[:etag] == secure_details[:etag]
-    content_length_match = details[:content_length] && details[:content_length] == secure_details[:content_length]
-    file_match = details[:file_hash] == secure_details[:file_hash]
-
-    return if !etag_match && !content_length_match && !file_match
-    problem "The URL #{url} could use HTTPS rather than HTTP"
-  end
-
   def problem(text)
     @problems << text
   end
-
-  def get_content_details(url)
-    out = {}
-    output, = curl_output "--connect-timeout", "15", "--include", url
-    split = output.partition("\r\n\r\n")
-    headers = split.first
-    out[:status] = headers[%r{HTTP\/.* (\d+)}, 1]
-    out[:etag] = headers[%r{ETag: ([wW]\/)?"(([^"]|\\")*)"}, 2]
-    out[:content_length] = headers[/Content-Length: (\d+)/, 1]
-    out[:file_hash] = Digest::SHA256.digest split.last
-    out
-  end
 end

Library/Homebrew/test/audit_test.rb

@@ -419,9 +419,8 @@ class FormulaAuditorTests < Homebrew::TestCase
     EOS
 
     fa.audit_homepage
-    assert_equal ["The homepage should start with http or https " \
-      "(URL is #{fa.formula.homepage}).", "The homepage #{fa.formula.homepage} is not reachable " \
-      "(HTTP status code 000)"], fa.problems
+    assert_equal ["The homepage should start with http or https (URL is #{fa.formula.homepage})."],
+      fa.problems
 
     formula_homepages = {
       "bar" => "http://www.freedesktop.org/wiki/bar",

Library/Homebrew/utils.rb

@@ -10,6 +10,7 @@ require "utils/github"
 require "utils/hash"
 require "utils/inreplace"
 require "utils/popen"
+require "utils/svn"
 require "utils/tty"
 require "time"
 

Library/Homebrew/utils/git.rb

@@ -40,4 +40,9 @@ module Utils
     @git_path = nil
     @git_version = nil
   end
+
+  def self.git_remote_exists(url)
+    return true unless git_available?
+    quiet_system "git", "ls-remote", url
+  end
 end

Library/Homebrew/utils/svn.rb

@@ -0,0 +1,11 @@
+module Utils
+  def self.svn_available?
+    return @svn if instance_variable_defined?(:@svn)
+    @svn = quiet_system HOMEBREW_SHIMS_PATH/"scm/svn", "--version"
+  end
+
+  def self.svn_remote_exists(url)
+    return true unless svn_available?
+    quiet_system "svn", "ls", url, "--depth", "empty"
+  end
+end