Merge pull request #24287 from Homebrew/require-official-formula-checksums

Require verifiable official formula sources

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)

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

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
 

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}

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]) }

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"

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"))