From 1a96dc39d1d74794de3216dc254e82702a48dddb Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sun, 7 May 2017 06:41:40 +0200 Subject: [PATCH 1/5] Add audit check to see if both version and checksum changed. --- Library/Homebrew/cask/lib/hbc/audit.rb | 24 ++++++++++++++-- Library/Homebrew/cask/lib/hbc/auditor.rb | 12 ++++---- .../hbc/cli/internal_audit_modified_casks.rb | 3 +- Library/Homebrew/utils/git.rb | 28 +++++++++++++++++++ 4 files changed, 59 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/audit.rb b/Library/Homebrew/cask/lib/hbc/audit.rb index 12cefb9395..8b88394c40 100644 --- a/Library/Homebrew/cask/lib/hbc/audit.rb +++ b/Library/Homebrew/cask/lib/hbc/audit.rb @@ -1,16 +1,18 @@ require "hbc/checkable" require "hbc/download" require "digest" +require "utils/git" module Hbc class Audit include Checkable - attr_reader :cask, :download + attr_reader :cask, :commit_range, :download - def initialize(cask, download: false, check_token_conflicts: false, command: SystemCommand) + def initialize(cask, download: false, check_token_conflicts: false, commit_range: nil, command: SystemCommand) @cask = cask @download = download + @commit_range = commit_range @check_token_conflicts = check_token_conflicts @command = command end @@ -21,6 +23,7 @@ module Hbc def run! check_required_stanzas + check_version_and_checksum check_version check_sha256 check_appcast @@ -57,6 +60,23 @@ module Hbc add_error "at least one activatable artifact stanza is required" if installable_artifacts.empty? end + def check_version_and_checksum + return if @cask.sourcefile_path.nil? + + tap = Tap.select { |t| t.cask_file?(@cask.sourcefile_path) }.first + return if tap.nil? + + previous_cask_contents = Git.last_revision_of_file(tap.path, @cask.sourcefile_path, before_commit: commit_range) + return if previous_cask_contents.empty? + + previous_cask = CaskLoader.load_from_string(previous_cask_contents) + + return unless previous_cask.version == cask.version + return if previous_cask.sha256 == cask.sha256 + + add_error "only sha256 changed; needs to be confirmed by the developer" + end + def check_version return unless cask.version check_no_string_version_latest diff --git a/Library/Homebrew/cask/lib/hbc/auditor.rb b/Library/Homebrew/cask/lib/hbc/auditor.rb index ec17f3cad6..48f36a54d5 100644 --- a/Library/Homebrew/cask/lib/hbc/auditor.rb +++ b/Library/Homebrew/cask/lib/hbc/auditor.rb @@ -1,14 +1,15 @@ module Hbc class Auditor - def self.audit(cask, audit_download: false, check_token_conflicts: false) - new(cask, audit_download, check_token_conflicts).audit + def self.audit(cask, audit_download: false, check_token_conflicts: false, commit_range: nil) + new(cask, audit_download, check_token_conflicts, commit_range).audit end - attr_reader :cask + attr_reader :cask, :commit_range - def initialize(cask, audit_download, check_token_conflicts) + def initialize(cask, audit_download, check_token_conflicts, commit_range) @cask = cask @audit_download = audit_download + @commit_range = commit_range @check_token_conflicts = check_token_conflicts end @@ -50,7 +51,8 @@ module Hbc def audit_cask_instance(cask) download = audit_download? && Download.new(cask) audit = Audit.new(cask, download: download, - check_token_conflicts: check_token_conflicts?) + check_token_conflicts: check_token_conflicts?, + commit_range: commit_range) audit.run! puts audit.summary audit.success? diff --git a/Library/Homebrew/cask/lib/hbc/cli/internal_audit_modified_casks.rb b/Library/Homebrew/cask/lib/hbc/cli/internal_audit_modified_casks.rb index 9467cccc71..1a8ca0e985 100644 --- a/Library/Homebrew/cask/lib/hbc/cli/internal_audit_modified_casks.rb +++ b/Library/Homebrew/cask/lib/hbc/cli/internal_audit_modified_casks.rb @@ -97,7 +97,8 @@ module Hbc audit_download = audit_download?(cask, cask_file) check_token_conflicts = added_cask_files.include?(cask_file) success = Auditor.audit(cask, audit_download: audit_download, - check_token_conflicts: check_token_conflicts) + check_token_conflicts: check_token_conflicts, + commit_range: commit_range) failed_casks << cask unless success end diff --git a/Library/Homebrew/utils/git.rb b/Library/Homebrew/utils/git.rb index 1b4d248946..43d93b64ec 100644 --- a/Library/Homebrew/utils/git.rb +++ b/Library/Homebrew/utils/git.rb @@ -1,3 +1,31 @@ +require "open3" + +module Git + module_function + + def last_revision_commit_of_file(repo, file, before_commit: nil) + args = [before_commit.nil? ? "--skip=1" : before_commit.split("..").first] + + out, = Open3.capture3( + HOMEBREW_SHIMS_PATH/"scm/git", "-C", repo, + "log", "--oneline", "--max-count=1", *args, "--", file + ) + out.split(" ").first + end + + def last_revision_of_file(repo, file, before_commit: nil) + relative_file = Pathname(file).relative_path_from(repo) + + commit_hash = last_revision_commit_of_file(repo, file, before_commit: before_commit) + + out, = Open3.capture3( + HOMEBREW_SHIMS_PATH/"scm/git", "-C", repo, + "show", "#{commit_hash}:#{relative_file}" + ) + out + end +end + module Utils def self.git_available? return @git if instance_variable_defined?(:@git) From ade62aa1b1c3c2eb68dc6f065687fe38a8846ab0 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 8 May 2017 17:45:00 +0200 Subject: [PATCH 2/5] Add the same check for Formulae. --- Library/Homebrew/dev-cmd/audit.rb | 19 +++++++++++++++++++ docs/Manpage.md | 3 +++ manpages/brew.1 | 3 +++ 3 files changed, 25 insertions(+) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 3c42b45a1d..c11c503e38 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -31,6 +31,9 @@ #: #: If `--except-cops` is passed, the given Rubocop cop(s)' checks would be skipped. #: +#: If `--commit-range` is is passed, the audited Formula will be compared to the +#: last revision before the ``. +#: #: `audit` exits with a non-zero status if any errors are found. This is useful, #: for instance, for implementing pre-commit hooks. @@ -648,9 +651,25 @@ class FormulaAuditor problem "Devel-only (no stable download)" end + previous_formula_contents = unless formula.tap.nil? + commit_range = ARGV.value("commit-range") + Git.last_revision_of_file(formula.tap.path, formula.path, before_commit: commit_range) + end + previous_formula = unless (previous_formula_contents || "").empty? + Formulary.from_contents(formula.name, formula.path, previous_formula_contents) + end + %w[Stable Devel HEAD].each do |name| next unless spec = formula.send(name.downcase) + unless previous_formula.nil? + previous_spec = previous_formula.send(name.downcase) + + if previous_spec.version == spec.version && previous_spec.checksum != spec.checksum + problem "#{name}: only sha256 changed; needs to be confirmed by the developer" + end + end + ra = ResourceAuditor.new(spec, online: @online, strict: @strict).audit problems.concat ra.problems.map { |problem| "#{name}: #{problem}" } diff --git a/docs/Manpage.md b/docs/Manpage.md index 2dac894434..fa8a7572a0 100644 --- a/docs/Manpage.md +++ b/docs/Manpage.md @@ -643,6 +643,9 @@ With `--verbose` or `-v`, many commands print extra debugging information. Note If `--except-cops` is passed, the given Rubocop cop(s)' checks would be skipped. + If `--commit-range` is is passed, the audited Formula will be compared to the + last revision before the ``commit_range``. + `audit` exits with a non-zero status if any errors are found. This is useful, for instance, for implementing pre-commit hooks. diff --git a/manpages/brew.1 b/manpages/brew.1 index ca11439a64..c4006ef470 100644 --- a/manpages/brew.1 +++ b/manpages/brew.1 @@ -674,6 +674,9 @@ If \fB\-\-only\-cops\fR is passed, only the given Rubocop cop(s)\' violations wo If \fB\-\-except\-cops\fR is passed, the given Rubocop cop(s)\' checks would be skipped\. . .IP +If \fB\-\-commit\-range\fR is is passed, the audited Formula will be compared to the last revision before the \fB\fR\. +. +.IP \fBaudit\fR exits with a non\-zero status if any errors are found\. This is useful, for instance, for implementing pre\-commit hooks\. . .TP From 330307b01a37ea514ec747ebab7a99dd47b79e7c Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 10 May 2017 20:45:34 +0200 Subject: [PATCH 3/5] Use `FormulaVersions` for checksum check. --- Library/Homebrew/dev-cmd/audit.rb | 28 +++++++++------------------- Library/Homebrew/formula_versions.rb | 20 ++++++++++++++++++++ docs/Manpage.md | 3 --- manpages/brew.1 | 3 --- 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index c11c503e38..4f71189c9b 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -31,9 +31,6 @@ #: #: If `--except-cops` is passed, the given Rubocop cop(s)' checks would be skipped. #: -#: If `--commit-range` is is passed, the audited Formula will be compared to the -#: last revision before the ``. -#: #: `audit` exits with a non-zero status if any errors are found. This is useful, #: for instance, for implementing pre-commit hooks. @@ -651,25 +648,9 @@ class FormulaAuditor problem "Devel-only (no stable download)" end - previous_formula_contents = unless formula.tap.nil? - commit_range = ARGV.value("commit-range") - Git.last_revision_of_file(formula.tap.path, formula.path, before_commit: commit_range) - end - previous_formula = unless (previous_formula_contents || "").empty? - Formulary.from_contents(formula.name, formula.path, previous_formula_contents) - end - %w[Stable Devel HEAD].each do |name| next unless spec = formula.send(name.downcase) - unless previous_formula.nil? - previous_spec = previous_formula.send(name.downcase) - - if previous_spec.version == spec.version && previous_spec.checksum != spec.checksum - problem "#{name}: only sha256 changed; needs to be confirmed by the developer" - end - end - ra = ResourceAuditor.new(spec, online: @online, strict: @strict).audit problems.concat ra.problems.map { |problem| "#{name}: #{problem}" } @@ -765,6 +746,15 @@ class FormulaAuditor return if @new_formula fv = FormulaVersions.new(formula) + + previous_version_and_checksum = fv.previous_version_and_checksum("origin/master") + [:stable, :devel].each do |spec_sym| + next unless spec = formula.send(spec_sym) + next unless previous_version_and_checksum[spec_sym][:version] == spec.version + next if previous_version_and_checksum[spec_sym][:checksum] == spec.checksum + problem "#{spec_sym}: only sha256 changed; needs to be confirmed by the developer" + end + attributes = [:revision, :version_scheme] attributes_map = fv.version_attributes_map(attributes, "origin/master") diff --git a/Library/Homebrew/formula_versions.rb b/Library/Homebrew/formula_versions.rb index 70706a2f01..5c4e0ecc96 100644 --- a/Library/Homebrew/formula_versions.rb +++ b/Library/Homebrew/formula_versions.rb @@ -63,6 +63,26 @@ class FormulaVersions map end + def previous_version_and_checksum(branch) + map = {} + + rev_list(branch) do |rev| + formula_at_revision(rev) do |f| + [:stable, :devel].each do |spec_sym| + next unless spec = f.send(spec_sym) + map[spec_sym] ||= { version: spec.version, checksum: spec.checksum } + end + + break if map[:stable] && map[:devel] + end + end + + map[:stable] ||= {} + map[:devel] ||= {} + + map + end + def version_attributes_map(attributes, branch) attributes_map = {} return attributes_map if attributes.empty? diff --git a/docs/Manpage.md b/docs/Manpage.md index fa8a7572a0..2dac894434 100644 --- a/docs/Manpage.md +++ b/docs/Manpage.md @@ -643,9 +643,6 @@ With `--verbose` or `-v`, many commands print extra debugging information. Note If `--except-cops` is passed, the given Rubocop cop(s)' checks would be skipped. - If `--commit-range` is is passed, the audited Formula will be compared to the - last revision before the ``commit_range``. - `audit` exits with a non-zero status if any errors are found. This is useful, for instance, for implementing pre-commit hooks. diff --git a/manpages/brew.1 b/manpages/brew.1 index c4006ef470..ca11439a64 100644 --- a/manpages/brew.1 +++ b/manpages/brew.1 @@ -674,9 +674,6 @@ If \fB\-\-only\-cops\fR is passed, only the given Rubocop cop(s)\' violations wo If \fB\-\-except\-cops\fR is passed, the given Rubocop cop(s)\' checks would be skipped\. . .IP -If \fB\-\-commit\-range\fR is is passed, the audited Formula will be compared to the last revision before the \fB\fR\. -. -.IP \fBaudit\fR exits with a non\-zero status if any errors are found\. This is useful, for instance, for implementing pre\-commit hooks\. . .TP From 798af254667f6488cc6a1bb554f423d88f2a7b22 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 10 May 2017 20:46:39 +0200 Subject: [PATCH 4/5] =?UTF-8?q?Don=E2=80=99t=20run=20checksum=20check=20if?= =?UTF-8?q?=20no=20commit=20range=20is=20given.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Library/Homebrew/cask/lib/hbc/audit.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/Library/Homebrew/cask/lib/hbc/audit.rb b/Library/Homebrew/cask/lib/hbc/audit.rb index 8b88394c40..6569da1264 100644 --- a/Library/Homebrew/cask/lib/hbc/audit.rb +++ b/Library/Homebrew/cask/lib/hbc/audit.rb @@ -66,6 +66,7 @@ module Hbc tap = Tap.select { |t| t.cask_file?(@cask.sourcefile_path) }.first return if tap.nil? + return if commit_range.nil? previous_cask_contents = Git.last_revision_of_file(tap.path, @cask.sourcefile_path, before_commit: commit_range) return if previous_cask_contents.empty? From 473bdadbcd0f87fdeda98f73b25bb47a14221281 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 22 May 2017 02:04:02 +0200 Subject: [PATCH 5/5] Change error messages. --- Library/Homebrew/cask/lib/hbc/audit.rb | 2 +- Library/Homebrew/dev-cmd/audit.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/audit.rb b/Library/Homebrew/cask/lib/hbc/audit.rb index 6569da1264..cee1fe8070 100644 --- a/Library/Homebrew/cask/lib/hbc/audit.rb +++ b/Library/Homebrew/cask/lib/hbc/audit.rb @@ -75,7 +75,7 @@ module Hbc return unless previous_cask.version == cask.version return if previous_cask.sha256 == cask.sha256 - add_error "only sha256 changed; needs to be confirmed by the developer" + add_error "only sha256 changed (see: https://github.com/caskroom/homebrew-cask/blob/master/doc/cask_language_reference/stanzas/sha256.md)" end def check_version diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 4f71189c9b..516388c688 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -752,7 +752,7 @@ class FormulaAuditor next unless spec = formula.send(spec_sym) next unless previous_version_and_checksum[spec_sym][:version] == spec.version next if previous_version_and_checksum[spec_sym][:checksum] == spec.checksum - problem "#{spec_sym}: only sha256 changed; needs to be confirmed by the developer" + problem "#{spec_sym}: sha256 changed without the version also changing; please create an issue upstream to rule out malicious circumstances and to find out why the file changed." end attributes = [:revision, :version_scheme]