Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

audit: Port audit_checksum method to rubocop and add tests #2755

Merged
merged 2 commits into from
Jun 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions Library/.rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ FormulaAudit/Text:
FormulaAudit/Caveats:
Enabled: true

FormulaAudit/Checksum:
Enabled: true

FormulaAuditStrict/BottleBlock:
Enabled: true

Expand Down
23 changes: 0 additions & 23 deletions Library/Homebrew/dev-cmd/audit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1246,7 +1246,6 @@ def initialize(resource, options = {})

def audit
audit_version
audit_checksum
audit_download_strategy
audit_urls
self
Expand All @@ -1273,28 +1272,6 @@ def audit_version
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 => :#{$1}` instead"
Expand Down
1 change: 1 addition & 0 deletions Library/Homebrew/rubocops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
require_relative "./rubocops/homepage_cop"
require_relative "./rubocops/text_cop"
require_relative "./rubocops/caveats_cop"
require_relative "./rubocops/checksum_cop"
55 changes: 55 additions & 0 deletions Library/Homebrew/rubocops/checksum_cop.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
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|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just make these lowercase, I don't think Devel looks very nice.

next unless spec_node = find_block(body_node, name.downcase.to_sym)
_, _, spec_body = *spec_node
audit_checksums(spec_body, name)
resource_blocks = find_all_blocks(spec_body, :resource)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to get all resource blocks that aren't inside a stable/devel/head.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not currently being done in audit_specs, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the resource blocks are currently being audited as part of running audit_specs, but the code structure relies on the ResourceAuditor class, and the audit_checksum it contains. I think that gap might be part of the cause of this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any existing method/code in audit.rb which audits resource blocks that aren't inside stable/devel/head blocks, is this something we want to add?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auditing the stable spec audits everything which is outside those blocks (as they are implicitly stable). The existing code is therefore fine, it's just this code that needs adjusted for that implicit stable resource handling. Make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Got it. Thanks.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to handle if this is nil; I hit this locally. Also, this doesn't seem to be constrained to the current node as far as I can tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should an error be raised, if there is no sha256 specified?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A RuboCop error: yes, I think so. I was seeing this on formulae that had them though so it may be a bug, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean, find_node_method_by_name returns nil even when there is sha256 in node?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember now helpfully but I think it's just that again we can have a stable/devel/head block that doesn't have a URL therefore doesn't need a checksum (although head urls don't need checksums either).

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-f0-9]+$/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return unless regex_match_group(checksum, /[A-F]/) seems correct for here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. This is embarrassing :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We all make mistakes; it's embarrassing that I didn't spot it in review!

problem "#{msg_prefix}sha256 should be lowercase"
end
end
end
end
end
11 changes: 8 additions & 3 deletions Library/Homebrew/rubocops/extend/formula_cop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,18 @@ def find_block(node, block_name)
nil
end

# Returns an array of block nodes named block_name inside node
# Returns an array of block nodes of depth first order named block_name below node
def find_blocks(node, block_name)
return if node.nil?
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?
Expand Down Expand Up @@ -250,8 +256,7 @@ def caveats_strings

# 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
Expand Down
78 changes: 78 additions & 0 deletions Library/Homebrew/test/rubocops/checksum_cop_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
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
end
end