From 4d7a98341556614818223060ca94413b17e3d1e6 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Thu, 1 Jun 2017 00:57:24 +0530 Subject: [PATCH 1/3] audit: Port audit_checksum method to rubocop and add tests --- Library/.rubocop.yml | 3 + Library/Homebrew/dev-cmd/audit.rb | 23 --- Library/Homebrew/rubocops.rb | 1 + Library/Homebrew/rubocops/checksum_cop.rb | 61 ++++++ .../Homebrew/rubocops/extend/formula_cop.rb | 9 +- .../test/rubocops/checksum_cop_spec.rb | 179 ++++++++++++++++++ 6 files changed, 251 insertions(+), 25 deletions(-) create mode 100644 Library/Homebrew/rubocops/checksum_cop.rb create mode 100644 Library/Homebrew/test/rubocops/checksum_cop_spec.rb diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 19d8714141..dafb1221b7 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -12,6 +12,9 @@ FormulaAudit/Text: FormulaAudit/Caveats: Enabled: true +FormulaAudit/Checksum: + Enabled: true + FormulaAuditStrict/BottleBlock: Enabled: true diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 0d9a630fdc..a6c71f795d 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -1246,7 +1246,6 @@ class ResourceAuditor def audit audit_version - audit_checksum audit_download_strategy audit_urls self @@ -1273,28 +1272,6 @@ class ResourceAuditor problem "version #{version} should not end with an underline and a number" end - def audit_checksum - return unless checksum - - case checksum.hash_type - when :md5 - problem "MD5 checksums are deprecated, please use SHA256" - return - when :sha1 - problem "SHA1 checksums are deprecated, please use SHA256" - return - when :sha256 then len = 64 - end - - if checksum.empty? - problem "#{checksum.hash_type} is empty" - else - problem "#{checksum.hash_type} should be #{len} characters" unless checksum.hexdigest.length == len - problem "#{checksum.hash_type} contains invalid characters" unless checksum.hexdigest =~ /^[a-fA-F0-9]+$/ - problem "#{checksum.hash_type} should be lowercase" unless checksum.hexdigest == checksum.hexdigest.downcase - end - end - def audit_download_strategy if url =~ %r{^(cvs|bzr|hg|fossil)://} || url =~ %r{^(svn)\+http://} problem "Use of the #{$&} scheme is deprecated, pass `:using => :#{Regexp.last_match(1)}` instead" diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb index c4a38cdb77..4710654fa2 100644 --- a/Library/Homebrew/rubocops.rb +++ b/Library/Homebrew/rubocops.rb @@ -5,3 +5,4 @@ require_relative "./rubocops/components_redundancy_cop" require_relative "./rubocops/homepage_cop" require_relative "./rubocops/text_cop" require_relative "./rubocops/caveats_cop" +require_relative "./rubocops/checksum_cop" diff --git a/Library/Homebrew/rubocops/checksum_cop.rb b/Library/Homebrew/rubocops/checksum_cop.rb new file mode 100644 index 0000000000..17ce1e83e0 --- /dev/null +++ b/Library/Homebrew/rubocops/checksum_cop.rb @@ -0,0 +1,61 @@ +require_relative "./extend/formula_cop" + +module RuboCop + module Cop + module FormulaAudit + class Checksum < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body_node) + %w[stable devel head].each do |name| + next unless spec_node = find_block(body_node, name.to_sym) + _, _, spec_body = *spec_node + audit_checksums(spec_body, name) + if name == "stable" + resource_blocks = find_blocks(body_node, :resource) + + find_all_blocks(spec_body, :resource) + else + resource_blocks = find_all_blocks(spec_body, :resource) + end + resource_blocks.each do |rb| + _, _, resource_body = *rb + audit_checksums(resource_body, name, string_content(parameters(rb).first)) + end + end + end + + def audit_checksums(node, spec, resource_name = nil) + msg_prefix = if resource_name + "#{spec} resource \"#{resource_name}\": " + else + "#{spec}: " + end + if find_node_method_by_name(node, :md5) + problem "#{msg_prefix}MD5 checksums are deprecated, please use SHA256" + end + + if find_node_method_by_name(node, :sha1) + problem "#{msg_prefix}SHA1 checksums are deprecated, please use SHA256" + end + + checksum_node = find_node_method_by_name(node, :sha256) + return if checksum_node.nil? + checksum = parameters(checksum_node).first + if string_content(checksum).size.zero? + problem "#{msg_prefix}sha256 is empty" + return + end + + if string_content(checksum).size != 64 && regex_match_group(checksum, /^\w*$/) + problem "#{msg_prefix}sha256 should be 64 characters" + end + + unless regex_match_group(checksum, /^[a-f0-9]+$/i) + problem "#{msg_prefix}sha256 contains invalid characters" + end + + return unless regex_match_group(checksum, /[A-F]+/) + problem "#{msg_prefix}sha256 should be lowercase" + end + end + end + end +end diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index 75a3e72d5d..98280841cc 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -173,6 +173,12 @@ module RuboCop node.each_child_node(:block).select { |block_node| block_name == block_node.method_name } end + # Returns an array of block nodes of any depth below node in AST + def find_all_blocks(node, block_name) + return if node.nil? + node.each_descendant(:block).select { |block_node| block_name == block_node.method_name } + end + # Returns a method definition node with method_name def find_method_def(node, method_name) return if node.nil? @@ -250,8 +256,7 @@ module RuboCop # Returns the array of arguments of the method_node def parameters(method_node) - return unless method_node.send_type? - method_node.method_args + method_node.method_args if method_node.send_type? || method_node.block_type? end # Returns true if the given parameters are present in method call diff --git a/Library/Homebrew/test/rubocops/checksum_cop_spec.rb b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb new file mode 100644 index 0000000000..c2caab73a5 --- /dev/null +++ b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb @@ -0,0 +1,179 @@ +require "rubocop" +require "rubocop/rspec/support" +require_relative "../../extend/string" +require_relative "../../rubocops/checksum_cop" + +describe RuboCop::Cop::FormulaAudit::Checksum do + subject(:cop) { described_class.new } + + context "When auditing spec checksums" do + it "When the checksum is empty" do + source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + stable do + url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" + sha256 "" + + resource "foo-package" do + url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" + sha256 "" + end + end + end + EOS + + expected_offenses = [{ message: "stable: sha256 is empty", + severity: :convention, + line: 5, + column: 4, + source: source }, + { message: "stable resource \"foo-package\": sha256 is empty", + severity: :convention, + line: 9, + column: 6, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When the checksum is not 64 characters" do + source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + stable do + url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426c0474cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9ad" + + resource "foo-package" do + url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426c047aaa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9" + end + end + end + EOS + + expected_offenses = [{ message: "stable: sha256 should be 64 characters", + severity: :convention, + line: 5, + column: 12, + source: source }, + { message: "stable resource \"foo-package\": sha256 should be 64 characters", + severity: :convention, + line: 9, + column: 14, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When the checksum has invalid chars" do + source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + stable do + url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426c0k7cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9a" + + resource "foo-package" do + url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426x047aa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea9" + end + end + end + EOS + + expected_offenses = [{ message: "stable: sha256 contains invalid characters", + severity: :convention, + line: 5, + column: 4, + source: source }, + { message: "stable resource \"foo-package\": sha256 contains invalid characters", + severity: :convention, + line: 9, + column: 6, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When the checksum has upper case characters" do + source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + stable do + url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0A645b426c0a7cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9a" + + resource "foo-package" do + url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" + sha256 "5cf6e1Ae0a645b426b047aa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea9" + end + end + end + EOS + + expected_offenses = [{ message: "stable: sha256 should be lowercase", + severity: :convention, + line: 5, + column: 21, + source: source }, + { message: "stable resource \"foo-package\": sha256 should be lowercase", + severity: :convention, + line: 9, + column: 20, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When auditing stable blocks outside spec blocks" do + source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + resource "foo-outside" do + url "https://github.com/foo-lang/foo-outside/archive/0.18.0.tar.gz" + sha256 "A4cc7cd3f7d1605ffa1ac5755cf6e1ae0a645b426b047a6a39a8b2268ddc7ea9" + end + stable do + url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426c0a7cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9a" + + resource "foo-package" do + url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426b047aa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea9" + end + end + end + EOS + + expected_offenses = [{ message: "stable resource \"foo-outside\": sha256 should be lowercase", + severity: :convention, + line: 5, + column: 12, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + end +end From 77da75e7d6ecc2ca749eb939d8e24ad9e3dfc5e1 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Wed, 14 Jun 2017 15:37:37 +0530 Subject: [PATCH 2/3] Simplify Checksum cop by auditing all checksums --- Library/Homebrew/rubocops/checksum_cop.rb | 63 +++++++++---------- .../test/rubocops/checksum_cop_spec.rb | 26 ++++---- 2 files changed, 41 insertions(+), 48 deletions(-) diff --git a/Library/Homebrew/rubocops/checksum_cop.rb b/Library/Homebrew/rubocops/checksum_cop.rb index 17ce1e83e0..d9e81a4eff 100644 --- a/Library/Homebrew/rubocops/checksum_cop.rb +++ b/Library/Homebrew/rubocops/checksum_cop.rb @@ -5,55 +5,48 @@ module RuboCop module FormulaAudit class Checksum < FormulaCop def audit_formula(_node, _class_node, _parent_class_node, body_node) - %w[stable devel head].each do |name| - next unless spec_node = find_block(body_node, name.to_sym) - _, _, spec_body = *spec_node - audit_checksums(spec_body, name) - if name == "stable" - resource_blocks = find_blocks(body_node, :resource) + - find_all_blocks(spec_body, :resource) - else - resource_blocks = find_all_blocks(spec_body, :resource) - end - resource_blocks.each do |rb| - _, _, resource_body = *rb - audit_checksums(resource_body, name, string_content(parameters(rb).first)) - end + return if body_node.nil? + if method_called_ever?(body_node, :md5) + problem "MD5 checksums are deprecated, please use SHA256" + end + + if method_called_ever?(body_node, :sha1) + problem "SHA1 checksums are deprecated, please use SHA256" + end + + sha256_calls = find_every_method_call_by_name(body_node, :sha256) + sha256_calls.each do |sha256_call| + sha256_node = get_checksum_node(sha256_call) + audit_sha256(sha256_node) end end - def audit_checksums(node, spec, resource_name = nil) - msg_prefix = if resource_name - "#{spec} resource \"#{resource_name}\": " - else - "#{spec}: " - end - if find_node_method_by_name(node, :md5) - problem "#{msg_prefix}MD5 checksums are deprecated, please use SHA256" + def get_checksum_node(call) + return if parameters(call).empty? || parameters(call).nil? + if parameters(call).first.str_type? + parameters(call).first + elsif parameters(call).first.hash_type? + parameters(call).first.keys.first end + end - if find_node_method_by_name(node, :sha1) - problem "#{msg_prefix}SHA1 checksums are deprecated, please use SHA256" - end - - checksum_node = find_node_method_by_name(node, :sha256) - return if checksum_node.nil? - checksum = parameters(checksum_node).first - if string_content(checksum).size.zero? - problem "#{msg_prefix}sha256 is empty" + def audit_sha256(checksum) + return if checksum.nil? + if regex_match_group(checksum, /^$/) + problem "sha256 is empty" return end if string_content(checksum).size != 64 && regex_match_group(checksum, /^\w*$/) - problem "#{msg_prefix}sha256 should be 64 characters" + problem "sha256 should be 64 characters" end - unless regex_match_group(checksum, /^[a-f0-9]+$/i) - problem "#{msg_prefix}sha256 contains invalid characters" + if regex_match_group(checksum, /[^a-f0-9]+/i) + problem "sha256 contains invalid characters" end return unless regex_match_group(checksum, /[A-F]+/) - problem "#{msg_prefix}sha256 should be lowercase" + problem "sha256 should be lowercase" end end end diff --git a/Library/Homebrew/test/rubocops/checksum_cop_spec.rb b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb index c2caab73a5..633f3117ac 100644 --- a/Library/Homebrew/test/rubocops/checksum_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb @@ -23,15 +23,15 @@ describe RuboCop::Cop::FormulaAudit::Checksum do end EOS - expected_offenses = [{ message: "stable: sha256 is empty", + expected_offenses = [{ message: "sha256 is empty", severity: :convention, line: 5, - column: 4, + column: 12, source: source }, - { message: "stable resource \"foo-package\": sha256 is empty", + { message: "sha256 is empty", severity: :convention, line: 9, - column: 6, + column: 14, source: source }] inspect_source(cop, source) @@ -57,12 +57,12 @@ describe RuboCop::Cop::FormulaAudit::Checksum do end EOS - expected_offenses = [{ message: "stable: sha256 should be 64 characters", + expected_offenses = [{ message: "sha256 should be 64 characters", severity: :convention, line: 5, column: 12, source: source }, - { message: "stable resource \"foo-package\": sha256 should be 64 characters", + { message: "sha256 should be 64 characters", severity: :convention, line: 9, column: 14, @@ -91,15 +91,15 @@ describe RuboCop::Cop::FormulaAudit::Checksum do end EOS - expected_offenses = [{ message: "stable: sha256 contains invalid characters", + expected_offenses = [{ message: "sha256 contains invalid characters", severity: :convention, line: 5, - column: 4, + column: 31, source: source }, - { message: "stable resource \"foo-package\": sha256 contains invalid characters", + { message: "sha256 contains invalid characters", severity: :convention, line: 9, - column: 6, + column: 31, source: source }] inspect_source(cop, source) @@ -125,12 +125,12 @@ describe RuboCop::Cop::FormulaAudit::Checksum do end EOS - expected_offenses = [{ message: "stable: sha256 should be lowercase", + expected_offenses = [{ message: "sha256 should be lowercase", severity: :convention, line: 5, column: 21, source: source }, - { message: "stable resource \"foo-package\": sha256 should be lowercase", + { message: "sha256 should be lowercase", severity: :convention, line: 9, column: 20, @@ -163,7 +163,7 @@ describe RuboCop::Cop::FormulaAudit::Checksum do end EOS - expected_offenses = [{ message: "stable resource \"foo-outside\": sha256 should be lowercase", + expected_offenses = [{ message: "sha256 should be lowercase", severity: :convention, line: 5, column: 12, From 0e1c88e7aefb93a8e2cc927fa9a4e903ac015c57 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Fri, 16 Jun 2017 19:44:14 +0530 Subject: [PATCH 3/3] Refactor Checksum cop to add autocorrect method --- Library/.rubocop.yml | 3 ++ Library/Homebrew/rubocops/checksum_cop.rb | 38 ++++++++++------ .../Homebrew/rubocops/extend/formula_cop.rb | 11 +++++ .../test/rubocops/checksum_cop_spec.rb | 43 +++++++++++++++++++ 4 files changed, 81 insertions(+), 14 deletions(-) diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index dafb1221b7..21f123859b 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -15,6 +15,9 @@ FormulaAudit/Caveats: FormulaAudit/Checksum: Enabled: true +FormulaAudit/ChecksumCase: + Enabled: true + FormulaAuditStrict/BottleBlock: Enabled: true diff --git a/Library/Homebrew/rubocops/checksum_cop.rb b/Library/Homebrew/rubocops/checksum_cop.rb index d9e81a4eff..dcaf60e7d3 100644 --- a/Library/Homebrew/rubocops/checksum_cop.rb +++ b/Library/Homebrew/rubocops/checksum_cop.rb @@ -21,15 +21,6 @@ module RuboCop end end - def get_checksum_node(call) - return if parameters(call).empty? || parameters(call).nil? - if parameters(call).first.str_type? - parameters(call).first - elsif parameters(call).first.hash_type? - parameters(call).first.keys.first - end - end - def audit_sha256(checksum) return if checksum.nil? if regex_match_group(checksum, /^$/) @@ -41,12 +32,31 @@ module RuboCop problem "sha256 should be 64 characters" end - if regex_match_group(checksum, /[^a-f0-9]+/i) - problem "sha256 contains invalid characters" - end + return unless regex_match_group(checksum, /[^a-f0-9]+/i) + problem "sha256 contains invalid characters" + end + end - return unless regex_match_group(checksum, /[A-F]+/) - problem "sha256 should be lowercase" + class ChecksumCase < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body_node) + return if body_node.nil? + sha256_calls = find_every_method_call_by_name(body_node, :sha256) + sha256_calls.each do |sha256_call| + checksum = get_checksum_node(sha256_call) + next if checksum.nil? + next unless regex_match_group(checksum, /[A-F]+/) + problem "sha256 should be lowercase" + end + end + + private + + def autocorrect(node) + lambda do |corrector| + correction = node.source.downcase + corrector.insert_before(node.source_range, correction) + corrector.remove(node.source_range) + end end end end diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index 98280841cc..439fde6a55 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -277,6 +277,17 @@ module RuboCop end end + # Returns the sha256 str node given a sha256 call node + def get_checksum_node(call) + return if parameters(call).empty? || parameters(call).nil? + if parameters(call).first.str_type? + parameters(call).first + # sha256 is passed as a key-value pair in bottle blocks + elsif parameters(call).first.hash_type? + parameters(call).first.keys.first + end + end + # Returns the begin position of the node's line in source code def line_start_column(node) node.source_range.source_buffer.line_range(node.loc.line).begin_pos diff --git a/Library/Homebrew/test/rubocops/checksum_cop_spec.rb b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb index 633f3117ac..644152c32c 100644 --- a/Library/Homebrew/test/rubocops/checksum_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb @@ -108,7 +108,13 @@ describe RuboCop::Cop::FormulaAudit::Checksum do expect_offense(expected, actual) end end + end +end +describe RuboCop::Cop::FormulaAudit::ChecksumCase do + subject(:cop) { described_class.new } + + context "When auditing spec checksums" do it "When the checksum has upper case characters" do source = <<-EOS.undent class Foo < Formula @@ -176,4 +182,41 @@ describe RuboCop::Cop::FormulaAudit::Checksum do end end end + + context "When auditing checksum with autocorrect" do + it "When there is uppercase sha256" do + source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + stable do + url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0A645b426c0a7cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9a" + + resource "foo-package" do + url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" + sha256 "5cf6e1Ae0a645b426b047aa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea9" + end + end + end + EOS + + corrected_source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + stable do + url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426c0a7cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9a" + + resource "foo-package" do + url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" + sha256 "5cf6e1ae0a645b426b047aa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea9" + end + end + end + EOS + + new_source = autocorrect_source(cop, source) + expect(new_source).to eq(corrected_source) + end + end end