Merge pull request #2911 from GauthamGoli/audit_urls_rubocop_part_1

audit: Port audit_urls partially to rubocop and add corresponding tests

4 files changed, 145 insertions, 49 deletions

diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb
index 4bcfdd128..327c3e811 100644
--- a/Library/Homebrew/dev-cmd/audit.rb
+++ b/Library/Homebrew/dev-cmd/audit.rb
@@ -1282,56 +1282,7 @@ class ResourceAuditor
   end
 
   def audit_urls
-    # Check GNU urls; doesn't apply to mirrors
-    if url =~ %r{^(?:https?|ftp)://ftpmirror.gnu.org/(.*)}
-      problem "Please use \"https://ftp.gnu.org/gnu/#{Regexp.last_match(1)}\" instead of #{url}."
-    end
-
-    # Fossies upstream requests they aren't used as primary URLs
-    # https://github.com/Homebrew/homebrew-core/issues/14486#issuecomment-307753234
-    if url =~ %r{^https?://fossies\.org/}
-      problem "Please don't use fossies.org in the url (using as a mirror is fine)"
-    end
-
-    if mirrors.include?(url)
-      problem "URL should not be duplicated as a mirror: #{url}"
-    end
-
     urls = [url] + mirrors
-
-    # Check a variety of SSL/TLS URLs that don't consistently auto-redirect
-    # or are overly common errors that need to be reduced & fixed over time.
-    urls.each do |p|
-      case p
-      when %r{^http://ftp\.gnu\.org/},
-           %r{^http://ftpmirror\.gnu\.org/},
-           %r{^http://download\.savannah\.gnu\.org/},
-           %r{^http://download-mirror\.savannah\.gnu\.org/},
-           %r{^http://[^/]*\.apache\.org/},
-           %r{^http://code\.google\.com/},
-           %r{^http://fossies\.org/},
-           %r{^http://mirrors\.kernel\.org/},
-           %r{^http://(?:[^/]*\.)?bintray\.com/},
-           %r{^http://tools\.ietf\.org/},
-           %r{^http://launchpad\.net/},
-           %r{^http://github\.com/},
-           %r{^http://bitbucket\.org/},
-           %r{^http://anonscm\.debian\.org/},
-           %r{^http://cpan\.metacpan\.org/},
-           %r{^http://hackage\.haskell\.org/},
-           %r{^http://(?:[^/]*\.)?archive\.org},
-           %r{^http://(?:[^/]*\.)?freedesktop\.org},
-           %r{^http://(?:[^/]*\.)?mirrorservice\.org/}
-        problem "Please use https:// for #{p}"
-      when %r{^http://search\.mcpan\.org/CPAN/(.*)}i
-        problem "#{p} should be `https://cpan.metacpan.org/#{Regexp.last_match(1)}`"
-      when %r{^(http|ftp)://ftp\.gnome\.org/pub/gnome/(.*)}i
-        problem "#{p} should be `https://download.gnome.org/#{Regexp.last_match(2)}`"
-      when %r{^git://anonscm\.debian\.org/users/(.*)}i
-        problem "#{p} should be `https://anonscm.debian.org/git/users/#{Regexp.last_match(1)}`"
-      end
-    end
-
     # Prefer HTTP/S when possible over FTP protocol due to possible firewalls.
     urls.each do |p|
       case p
diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb
index f673720a6..7d104ad9e 100644
--- a/Library/Homebrew/rubocops.rb
+++ b/Library/Homebrew/rubocops.rb
@@ -9,3 +9,4 @@ require_relative "./rubocops/checksum_cop"
 require_relative "./rubocops/legacy_patches_cop"
 require_relative "./rubocops/conflicts_cop"
 require_relative "./rubocops/options_cop"
+require_relative "./rubocops/urls_cop"
diff --git a/Library/Homebrew/rubocops/urls_cop.rb b/Library/Homebrew/rubocops/urls_cop.rb
new file mode 100644
index 000000000..830b68ead
--- /dev/null
+++ b/Library/Homebrew/rubocops/urls_cop.rb
@@ -0,0 +1,90 @@
+require_relative "./extend/formula_cop"
+
+module RuboCop
+  module Cop
+    module FormulaAudit
+      # This cop audits urls and mirrors in Formulae
+      class Urls < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, body_node)
+          urls = find_every_method_call_by_name(body_node, :url)
+          mirrors = find_every_method_call_by_name(body_node, :mirror)
+
+          # GNU urls; doesn't apply to mirrors
+          gnu_pattern = %r{^(?:https?|ftp)://ftpmirror.gnu.org/(.*)}
+          audit_urls(urls, gnu_pattern) do |match, url|
+            problem "Please use \"https://ftp.gnu.org/gnu/#{match[1]}\" instead of #{url}."
+          end
+
+          # Fossies upstream requests they aren't used as primary URLs
+          # https://github.com/Homebrew/homebrew-core/issues/14486#issuecomment-307753234
+          fossies_pattern = %r{^https?://fossies\.org/}
+          audit_urls(urls, fossies_pattern) do
+            problem "Please don't use fossies.org in the url (using as a mirror is fine)"
+          end
+
+          audit_urls(mirrors, /.*/) do |_, mirror|
+            urls.each do |url|
+              url_string = string_content(parameters(url).first)
+              next unless url_string.eql?(mirror)
+              problem "URL should not be duplicated as a mirror: #{url_string}"
+            end
+          end
+
+          urls += mirrors
+
+          # Check a variety of SSL/TLS URLs that don't consistently auto-redirect
+          # or are overly common errors that need to be reduced & fixed over time.
+          http_to_https_patterns = Regexp.union([%r{^http://ftp\.gnu\.org/},
+                                                 %r{^http://ftpmirror\.gnu\.org/},
+                                                 %r{^http://download\.savannah\.gnu\.org/},
+                                                 %r{^http://download-mirror\.savannah\.gnu\.org/},
+                                                 %r{^http://[^/]*\.apache\.org/},
+                                                 %r{^http://code\.google\.com/},
+                                                 %r{^http://fossies\.org/},
+                                                 %r{^http://mirrors\.kernel\.org/},
+                                                 %r{^http://(?:[^/]*\.)?bintray\.com/},
+                                                 %r{^http://tools\.ietf\.org/},
+                                                 %r{^http://launchpad\.net/},
+                                                 %r{^http://github\.com/},
+                                                 %r{^http://bitbucket\.org/},
+                                                 %r{^http://anonscm\.debian\.org/},
+                                                 %r{^http://cpan\.metacpan\.org/},
+                                                 %r{^http://hackage\.haskell\.org/},
+                                                 %r{^http://(?:[^/]*\.)?archive\.org},
+                                                 %r{^http://(?:[^/]*\.)?freedesktop\.org},
+                                                 %r{^http://(?:[^/]*\.)?mirrorservice\.org/}])
+          audit_urls(urls, http_to_https_patterns) do |_, url|
+            problem "Please use https:// for #{url}"
+          end
+
+          cpan_pattern = %r{^http://search\.mcpan\.org/CPAN/(.*)}i
+          audit_urls(urls, cpan_pattern) do |match, url|
+            problem "#{url} should be `https://cpan.metacpan.org/#{match[1]}`"
+          end
+
+          gnome_pattern = %r{^(http|ftp)://ftp\.gnome\.org/pub/gnome/(.*)}i
+          audit_urls(urls, gnome_pattern) do |match, url|
+            problem "#{url} should be `https://download.gnome.org/#{match[2]}`"
+          end
+
+          debian_pattern = %r{^git://anonscm\.debian\.org/users/(.*)}i
+          audit_urls(urls, debian_pattern) do |match, url|
+            problem "#{url} should be `https://anonscm.debian.org/git/users/#{match[1]}`"
+          end
+        end
+
+        private
+
+        def audit_urls(urls, regex)
+          urls.each do |url_node|
+            url_string_node = parameters(url_node).first
+            url_string = string_content(url_string_node)
+            match_object = regex_match_group(url_string_node, regex)
+            offending_node(url_string_node.parent) if match_object
+            yield match_object, url_string if match_object
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/Library/Homebrew/test/rubocops/urls_cop_spec.rb b/Library/Homebrew/test/rubocops/urls_cop_spec.rb
new file mode 100644
index 000000000..2e56dbf03
--- /dev/null
+++ b/Library/Homebrew/test/rubocops/urls_cop_spec.rb
@@ -0,0 +1,54 @@
+require "rubocop"
+require "rubocop/rspec/support"
+require_relative "../../extend/string"
+require_relative "../../rubocops/urls_cop"
+
+describe RuboCop::Cop::FormulaAudit::Urls do
+  subject(:cop) { described_class.new }
+
+  context "When auditing urls" do
+    it "with offenses" do
+      formulas = [{
+        "url" => "https://ftpmirror.gnu.org/lightning/lightning-2.1.0.tar.gz",
+        "msg" => 'Please use "https://ftp.gnu.org/gnu/lightning/lightning-2.1.0.tar.gz" instead of https://ftpmirror.gnu.org/lightning/lightning-2.1.0.tar.gz.',
+        "col" => 2,
+      }, {
+        "url" => "https://fossies.org/linux/privat/monit-5.23.0.tar.gz",
+        "msg" => "Please don't use fossies.org in the url (using as a mirror is fine)",
+        "col" => 2,
+      }, {
+        "url" => "http://tools.ietf.org/tools/rfcmarkup/rfcmarkup-1.119.tgz",
+        "msg" => "Please use https:// for http://tools.ietf.org/tools/rfcmarkup/rfcmarkup-1.119.tgz",
+        "col" => 2,
+      }, {
+        "url" => "http://search.mcpan.org/CPAN/authors/id/Z/ZE/ZEFRAM/Perl4-CoreLibs-0.003.tar.gz",
+        "msg" => "http://search.mcpan.org/CPAN/authors/id/Z/ZE/ZEFRAM/Perl4-CoreLibs-0.003.tar.gz should be `https://cpan.metacpan.org/authors/id/Z/ZE/ZEFRAM/Perl4-CoreLibs-0.003.tar.gz`",
+        "col" => 2,
+      }, {
+        "url" => "http://ftp.gnome.org/pub/GNOME/binaries/mac/banshee/banshee-2.macosx.intel.dmg",
+        "msg" => "http://ftp.gnome.org/pub/GNOME/binaries/mac/banshee/banshee-2.macosx.intel.dmg should be `https://download.gnome.org/binaries/mac/banshee/banshee-2.macosx.intel.dmg`",
+        "col" => 2,
+      }]
+
+      formulas.each do |formula|
+        source = <<-EOS.undent
+          class Foo < Formula
+            desc "foo"
+            url "#{formula["url"]}"
+          end
+        EOS
+        expected_offenses = [{ message: formula["msg"],
+                               severity: :convention,
+                               line: 3,
+                               column: formula["col"],
+                               source: source }]
+
+        inspect_source(cop, source)
+
+        expected_offenses.zip(cop.offenses.reverse).each do |expected, actual|
+          expect_offense(expected, actual)
+        end
+      end
+    end
+  end
+end