diff --git a/Library/Homebrew/download_strategy/vcs_download_strategy.rb b/Library/Homebrew/download_strategy/vcs_download_strategy.rb index a43a327118..e10a4969a4 100644 --- a/Library/Homebrew/download_strategy/vcs_download_strategy.rb +++ b/Library/Homebrew/download_strategy/vcs_download_strategy.rb @@ -15,6 +15,10 @@ class VCSDownloadStrategy < AbstractDownloadStrategy sig { params(url: String, name: String, version: T.nilable(T.any(String, Version)), meta: T.untyped).void } def initialize(url, name, version, **meta) super + # Deprecate stable Subversion, CVS, Mercurial and Fossil sources in the next major release. + # if !head? && !is_a?(GitDownloadStrategy) + # odeprecated self.class.to_s, "GitDownloadStrategy or CurlDownloadStrategy", disable: true + # end extracted_ref = extract_ref(meta) @ref_type = T.let(extracted_ref.fetch(0), T.nilable(Symbol)) @ref = T.let(extracted_ref.fetch(1), T.untyped) diff --git a/Library/Homebrew/external_patch.rb b/Library/Homebrew/external_patch.rb index bef8a4e2ef..6677bbb716 100644 --- a/Library/Homebrew/external_patch.rb +++ b/Library/Homebrew/external_patch.rb @@ -53,7 +53,7 @@ class ExternalPatch def apply begin if downloaded? - verify_download_integrity(cached_download) if resource.checksum.present? + verify_download_integrity(cached_download) else fetch end diff --git a/Library/Homebrew/formula_auditor.rb b/Library/Homebrew/formula_auditor.rb index 54ea578b28..28d96333fc 100644 --- a/Library/Homebrew/formula_auditor.rb +++ b/Library/Homebrew/formula_auditor.rb @@ -917,7 +917,7 @@ module Homebrew ra = ResourceAuditor.new( resource, spec_name, - online: @online, strict: @strict, only: @only, except: @except, + online: @online, strict: @strict, only: @only, except: @except, core_tap: @core_tap, use_homebrew_curl: resource.using == :homebrew_curl ).audit ra.problems.each do |message| @@ -925,6 +925,16 @@ module Homebrew end end + (spec.patches + spec.resources.values.flat_map(&:patches)).grep(ExternalPatch).each do |patch| + ra = ResourceAuditor.new(patch.resource, spec_name, core_tap: @core_tap) + %w[checksum download_strategy].freeze.each do |audit| + next if @except&.include?(audit) || (@only.present? && @only.exclude?(audit)) + + ra.public_send(:"audit_#{audit}") + end + ra.problems.each { |message| problem "#{name} patch: #{message}" } + end + next if spec.patches.empty? next if !@new_formula || !@core_tap diff --git a/Library/Homebrew/resource.rb b/Library/Homebrew/resource.rb index 465b13e857..116189fdb8 100644 --- a/Library/Homebrew/resource.rb +++ b/Library/Homebrew/resource.rb @@ -114,7 +114,7 @@ class Resource begin if !downloaded? fetch - elsif !staged && checksum.present? + elsif !staged verify_download_integrity(cached_download) end rescue ChecksumMismatchError @@ -224,6 +224,27 @@ class Resource super(verify_download_integrity:, timeout:, quiet:) end + sig { override.params(filename: Pathname).void } + def verify_download_integrity(filename) + if filename.file? && checksum.blank? && !Homebrew::EnvConfig.no_install_from_api? + spec = T.let(owner, T.nilable(Owner)) + spec = spec.owner while spec.is_a?(Resource) + formula = spec.is_a?(SoftwareSpec) ? spec.owner : spec + return super if formula.is_a?(::Formula) && spec.equal?(formula.head) && version&.head? + + if formula.is_a?(::Formula) && formula.tap&.official? + raise ChecksumMissingError, "#{formula.full_name}: a SHA-256 checksum is required for #{url}." + end + + # Enable for third-party taps in the next major release. + # if formula.is_a?(::Formula) && formula.tap + # odeprecated "formula downloads without a checksum", "a `sha256` declaration", disable: true + # end + end + + super + end + # {Livecheck} can be used to check for newer versions of the software. # This method evaluates the DSL specified in the `livecheck` block of the # {Resource} (if it exists) and sets the instance variables of a {Livecheck} diff --git a/Library/Homebrew/resource_auditor.rb b/Library/Homebrew/resource_auditor.rb index 1d0056064d..c122a93c2a 100644 --- a/Library/Homebrew/resource_auditor.rb +++ b/Library/Homebrew/resource_auditor.rb @@ -111,6 +111,9 @@ module Homebrew if (using == :git || url_strategy == GitDownloadStrategy) && specs[:tag] && !specs[:revision] problem "Git should specify `revision:` when a `tag:` is specified." + elsif !version&.head? && DownloadStrategyDetector.detect(url!, using) <= GitDownloadStrategy && + !specs[:revision].to_s.match?(/\A(?:[0-9a-f]{40}|[0-9a-f]{64})\z/i) + problem "Git should specify a full commit hash in `revision:`." end return unless using @@ -138,13 +141,21 @@ module Homebrew sig { void } def audit_checksum - return if spec_name == :head - # This condition is non-invertible. - # rubocop:disable Style/InvertibleUnlessCondition - return unless DownloadStrategyDetector.detect(url.to_s, using) <= CurlDownloadStrategy - # rubocop:enable Style/InvertibleUnlessCondition + return if spec_name == :head && version&.head? - problem "Checksum is missing" if checksum.blank? + strategy = DownloadStrategyDetector.detect(url.to_s, using) + if strategy <= AbstractFileDownloadStrategy + problem "Checksum is missing" if checksum.blank? + return + end + return if strategy <= GitDownloadStrategy + + formula = T.let(owner, T.nilable(Resource::Owner)) + formula = formula.owner while formula.is_a?(SoftwareSpec) || formula.is_a?(Resource) + # Remove this restriction with the next-major download strategy deprecations. + return if !@core_tap && (!formula.is_a?(::Formula) || !formula.tap&.official?) + + problem "Use a checksummed archive or a Git commit." end sig { returns(T::Array[String]) } diff --git a/Library/Homebrew/test/formula_auditor_spec.rb b/Library/Homebrew/test/formula_auditor_spec.rb index 516bd2f821..b3f2972f4a 100644 --- a/Library/Homebrew/test/formula_auditor_spec.rb +++ b/Library/Homebrew/test/formula_auditor_spec.rb @@ -916,6 +916,76 @@ RSpec.describe Homebrew::FormulaAuditor do expect(fa.problems).to be_empty end + %w[svn cvs hg fossil].each do |strategy| + it "rejects stable #{strategy} sources in official taps" do + fa = formula_auditor "foo", <<~RUBY, core_tap: true, only: ["checksum"] + class Foo < Formula + url "#{strategy}://brew.sh/foo", revision: "1234" + version "1.0" + sha256 "#{TEST_SHA256}" + end + RUBY + + fa.audit_specs + expect(fa.problems.map { |problem| problem[:message] }) + .to include("Stable: Use a checksummed archive or a Git commit.") + end + end + + it "checks stable sources in official taps other than core" do + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + url "svn://brew.sh/foo", revision: "1234" + version "1.0" + end + RUBY + allow(fa.formula).to receive(:tap).and_return(Tap.fetch("homebrew/test")) + + fa.audit_specs + expect(fa.problems.map { |problem| problem[:message] }) + .to include("Stable: Use a checksummed archive or a Git commit.") + end + + it "checks nested resources in official taps" do + fa = formula_auditor "foo", <<~RUBY, core_tap: true, only: ["checksum"] + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + sha256 "#{TEST_SHA256}" + resource "bar" do + url "svn://brew.sh/bar", revision: "1234" + version "1.0" + end + head do + url "svn://brew.sh/foo" + resource "baz" do + url "hg://brew.sh/baz", revision: "1234" + version "1.0" + end + end + end + RUBY + + fa.audit_specs + expect(fa.problems.map { |problem| problem[:message] }).to contain_exactly( + 'Stable resource "bar": Use a checksummed archive or a Git commit.', + 'HEAD resource "bar": Use a checksummed archive or a Git commit.', + 'HEAD resource "baz": Use a checksummed archive or a Git commit.', + ) + end + + it "continues to allow SVN in unofficial taps" do + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + url "svn://brew.sh/foo", revision: "1234" + version "1.0" + end + RUBY + allow(fa.formula).to receive(:tap).and_return(Tap.fetch("test/tap")) + + fa.audit_specs + expect(fa.problems).to be_empty + end + it "allows to miss a checksum for HEAD" do fa = formula_auditor "foo", <<~RUBY class Foo < Formula @@ -929,6 +999,94 @@ RSpec.describe Homebrew::FormulaAuditor do expect(fa.problems).to be_empty end + it "requires checksums for versioned resources in HEAD builds" do + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + head do + url "https://brew.sh/foo.git" + resource "bar" do + url "https://brew.sh/bar-1.0.tar.gz" + end + end + end + RUBY + + fa.audit_specs + expect(fa.problems.map { |problem| problem[:message] }) + .to include('HEAD resource "bar": Checksum is missing') + end + + it "requires checksums for formula and resource patches in third-party taps" do + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + sha256 "#{TEST_SHA256}" + patch do + url "https://brew.sh/foo.diff" + end + resource "bar" do + url "https://brew.sh/bar-1.0.tgz" + sha256 "#{TEST_SHA256}" + patch do + url "https://brew.sh/bar.diff" + end + end + end + RUBY + + fa.audit_specs + expect(fa.problems.map { |problem| problem[:message] }).to contain_exactly( + "Stable patch: Checksum is missing", "Stable patch: Checksum is missing" + ) + end + + [ + [{ core_tap: true }, nil, 2], + [{}, nil, 2], + [{}, "abc1234", 2], + [{}, "a" * 40, 0], + [{ only: ["download_strategy"] }, nil, 2], + [{ except: ["checksum"] }, nil, 2], + [{ only: ["checksum"] }, nil, 0], + [{ except: ["download_strategy"] }, nil, 0], + ].each do |options, revision, problem_count| + it "audits formula and resource Git patches with #{options} and revision #{revision.inspect}" do + fa = formula_auditor "foo", <<~RUBY, options + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + sha256 "#{TEST_SHA256}" + patch do + url "https://brew.sh/foo-patches", using: :git, revision: #{revision.inspect} + end + resource "bar" do + url "https://brew.sh/bar-1.0.tgz" + sha256 "#{TEST_SHA256}" + patch do + url "https://brew.sh/bar-patches", using: :git, revision: #{revision.inspect} + end + end + end + RUBY + + fa.audit_specs + expect(fa.problems.map { |problem| problem[:message] }) + .to eq(Array.new(problem_count, "Stable patch: Git should specify a full commit hash in `revision:`.")) + end + end + + it "requires full Git revisions for stable third-party downloads" do + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + url "https://brew.sh/foo.git", revision: "abc1234" + version "1.0" + end + RUBY + + fa.audit_specs + expect(fa.problems.map { |problem| problem[:message] }) + .to include("Stable: Git should specify a full commit hash in `revision:`.") + end + it "accepts a curl dependency with a working HTTP mirror" do sha256 = "31cccfc6630528db1c8e3a06f6decf2a370060b982841cfab2b8677400a5092e" mirror_url = "http://brew.sh/mirror/foo-1.0.tgz" diff --git a/Library/Homebrew/test/resource_spec.rb b/Library/Homebrew/test/resource_spec.rb index b555d37082..b546ccfb09 100644 --- a/Library/Homebrew/test/resource_spec.rb +++ b/Library/Homebrew/test/resource_spec.rb @@ -247,6 +247,20 @@ RSpec.describe Resource do expect { resource.stage(mktmpdir) }.not_to raise_error end + it "rejects a cached official formula resource without a checksum" do + ENV.delete("HOMEBREW_NO_INSTALL_FROM_API") + f = formula do + T.bind(self, T.class_of(Formula)) + url "foo-1.0.tar.gz" + end + allow(f).to receive(:tap).and_return(CoreTap.instance) + resource.owner = f + resource.checksum = nil + resource.downloader.fetch + + expect { resource.stage(mktmpdir) }.to raise_error(ChecksumMissingError) + end + it "keeps the recorded modification time out of a staged git checkout's status" do mktmpdir do |staging_path| (staging_path/".git").mkpath @@ -367,6 +381,101 @@ RSpec.describe Resource do end.to output(/Cannot verify integrity/).to_stderr end + describe "official formula checksums" do + let(:f) do + formula do + T.bind(self, T.class_of(Formula)) + url "foo-1.0.tar.gz" + resource "bar" do + url "bar-1.0.tar.gz" + end + patch do + url "https://brew.sh/foo.diff" + end + head do + url "https://brew.sh/foo.tar.gz" + resource "moving" do + url "https://brew.sh/latest.tar.gz" + end + resource "versioned" do + url "https://brew.sh/versioned-1.0.tar.gz" + end + end + end + end + let(:download) { mktmpdir/"source.tar.gz" } + + before do + ENV.delete("HOMEBREW_NO_INSTALL_FROM_API") + allow(f).to receive(:tap).and_return(CoreTap.instance) + download.write("content") + end + + it "rejects a stable source archive without a checksum" do + expect { f.active_spec.verify_download_integrity(download) }.to raise_error(ChecksumMissingError) + end + + it "rejects a resource without a checksum" do + expect { f.resource("bar").verify_download_integrity(download) }.to raise_error(ChecksumMissingError) + end + + it "rejects a cached external patch without a checksum" do + patch = f.patchlist.fetch(0) + raise "Expected an external patch" unless patch.is_a?(ExternalPatch) + + allow(patch).to receive_messages(downloaded?: true, cached_download: download) + allow(patch.resource).to receive(:unpack) + + expect { patch.apply }.to raise_error(ChecksumMissingError) + end + + it "keeps the development override for missing checksums" do + ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" + + expect { f.active_spec.verify_download_integrity(download) }.to output(/Cannot verify integrity/).to_stderr + end + + it "still rejects mismatches with the development override" do + ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" + f.resource("bar").sha256(TEST_SHA256) + + expect { f.resource("bar").verify_download_integrity(download) }.to raise_error(ChecksumMismatchError) + end + + it "warns for third-party formulae until the next major release" do + allow(f).to receive(:tap).and_return(Tap.fetch("test/tap")) + + expect { f.active_spec.verify_download_integrity(download) }.to output(/Cannot verify integrity/).to_stderr + end + + it "allows the moving HEAD source archive" do + expect { f.head&.resource&.verify_download_integrity(download) }.to output(/Cannot verify integrity/).to_stderr + end + + it "allows a moving HEAD resource archive" do + expect { f.head&.resource("moving")&.verify_download_integrity(download) } + .to output(/Cannot verify integrity/).to_stderr + end + + it "rejects a versioned HEAD resource archive without a checksum" do + expect do + f.head&.resource("versioned")&.verify_download_integrity(download) + end.to raise_error(ChecksumMissingError) + end + + it "still rejects mismatches for moving HEAD resources" do + f.head&.resource("moving")&.sha256(TEST_SHA256) + + expect { f.head&.resource("moving")&.verify_download_integrity(download) }.to raise_error(ChecksumMismatchError) + end + + it "requires checksums for stable resources with a HEAD version" do + f.resource("bar").version("HEAD") + + expect { f.resource("bar").verify_download_integrity(download) }.to raise_error(ChecksumMissingError) + end + end + specify "#verify_download_integrity_mismatch" do fn = Pathname.new("foo") allow(fn).to receive_messages(file?: true, sha256: Digest::SHA256.hexdigest("mismatch"))