From 4f469234d0a6a9a9c78850760fe978bbf2c1a80e Mon Sep 17 00:00:00 2001
From: Sam Ford <1584702+samford@users.noreply.github.com>
Date: Wed, 8 Nov 2023 09:06:24 -0500
Subject: [PATCH 1/6] Sparkle: Ensure empty strings become nil

Sometimes appcasts contain empty elements/attributes and the `Item`
values end up as an empty string because of how they're handled in
`#items_from_content`. It's reasonable to expect that empty values
would be `nil` instead, so this adds `#presence` calls to ensure this
is the case.
---
 .../Homebrew/livecheck/strategy/sparkle.rb    | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Library/Homebrew/livecheck/strategy/sparkle.rb b/Library/Homebrew/livecheck/strategy/sparkle.rb
index 9f52b8b88b..cd400c81a2 100644
--- a/Library/Homebrew/livecheck/strategy/sparkle.rb
+++ b/Library/Homebrew/livecheck/strategy/sparkle.rb
@@ -86,19 +86,19 @@ module Homebrew
             enclosure = item.elements["enclosure"]
 
             if enclosure
-              url = enclosure["url"]
-              short_version = enclosure["shortVersionString"]
-              version = enclosure["version"]
-              os = enclosure["os"]
+              url = enclosure["url"].presence
+              short_version = enclosure["shortVersionString"].presence
+              version = enclosure["version"].presence
+              os = enclosure["os"].presence
             end
 
-            title = item.elements["title"]&.text&.strip
-            link = item.elements["link"]&.text&.strip
+            title = item.elements["title"]&.text&.strip&.presence
+            link = item.elements["link"]&.text&.strip&.presence
             url ||= link
-            channel = item.elements["channel"]&.text&.strip
-            release_notes_link = item.elements["releaseNotesLink"]&.text&.strip
-            short_version ||= item.elements["shortVersionString"]&.text&.strip
-            version ||= item.elements["version"]&.text&.strip
+            channel = item.elements["channel"]&.text&.strip&.presence
+            release_notes_link = item.elements["releaseNotesLink"]&.text&.strip&.presence
+            short_version ||= item.elements["shortVersionString"]&.text&.strip&.presence
+            version ||= item.elements["version"]&.text&.strip&.presence
 
             pub_date = item.elements["pubDate"]&.text&.strip&.presence&.then do |date_string|
               Time.parse(date_string)

From bc2ce97e5de8c7cce272aa66521f17d7af38a3ed Mon Sep 17 00:00:00 2001
From: Sam Ford <1584702+samford@users.noreply.github.com>
Date: Wed, 8 Nov 2023 09:10:09 -0500
Subject: [PATCH 2/6] Sparkle: Move sorting/filtering into methods

We need to be able to replicate the `Sparkle` strategy's sorting
and filtering behavior in a related cask audit, so this extracts
the logic into reusable methods.

This also stores `item.minimum_system_version` as a `MacOSVersion`
object (instead of a string), so we can do proper version comparison
(instead of naive string comparison) wherever needed.
---
 .../Homebrew/livecheck/strategy/sparkle.rb    | 51 +++++++++---
 .../test/livecheck/strategy/sparkle_spec.rb   | 79 ++++++++++++++++++-
 2 files changed, 113 insertions(+), 17 deletions(-)

diff --git a/Library/Homebrew/livecheck/strategy/sparkle.rb b/Library/Homebrew/livecheck/strategy/sparkle.rb
index cd400c81a2..1d6b32cf40 100644
--- a/Library/Homebrew/livecheck/strategy/sparkle.rb
+++ b/Library/Homebrew/livecheck/strategy/sparkle.rb
@@ -100,6 +100,16 @@ module Homebrew
             short_version ||= item.elements["shortVersionString"]&.text&.strip&.presence
             version ||= item.elements["version"]&.text&.strip&.presence
 
+            minimum_system_version_text =
+              item.elements["minimumSystemVersion"]&.text&.strip&.gsub(/\A\D+|\D+\z/, "")&.presence
+            if minimum_system_version_text.present?
+              minimum_system_version = begin
+                MacOSVersion.new(minimum_system_version_text)
+              rescue MacOSVersion::Error
+                nil
+              end
+            end
+
             pub_date = item.elements["pubDate"]&.text&.strip&.presence&.then do |date_string|
               Time.parse(date_string)
             rescue ArgumentError
@@ -114,18 +124,6 @@ module Homebrew
 
             bundle_version = BundleVersion.new(short_version, version) if short_version || version
 
-            next if os && !((os == "osx") || (os == "macos"))
-
-            if (minimum_system_version = item.elements["minimumSystemVersion"]&.text&.gsub(/\A\D+|\D+\z/, ""))
-              macos_minimum_system_version = begin
-                MacOSVersion.new(minimum_system_version).strip_patch
-              rescue MacOSVersion::Error
-                nil
-              end
-
-              next if macos_minimum_system_version&.prerelease?
-            end
-
             data = {
               title:                  title,
               link:                   link,
@@ -146,6 +144,33 @@ module Homebrew
           end.compact
         end
 
+        # Filters out items that aren't suitable for Homebrew.
+        #
+        # @param items [Array] appcast items
+        # @return [Array]
+        sig { params(items: T::Array[Item]).returns(T::Array[Item]) }
+        def self.filter_items(items)
+          items.select do |item|
+            # Omit items with an explicit `os` value that isn't macOS
+            next false if item.os && !((item.os == "osx") || (item.os == "macos"))
+
+            # Omit items for prerelease macOS versions
+            next false if item.minimum_system_version&.strip_patch&.prerelease?
+
+            true
+          end.compact
+        end
+
+        # Sorts items from newest to oldest.
+        #
+        # @param items [Array] appcast items
+        # @return [Array]
+        sig { params(items: T::Array[Item]).returns(T::Array[Item]) }
+        def self.sort_items(items)
+          items.sort_by { |item| [item.pub_date, item.bundle_version] }
+               .reverse
+        end
+
         # Uses `#items_from_content` to identify versions from the Sparkle
         # appcast content or, if a block is provided, passes the content to
         # the block to handle matching.
@@ -161,7 +186,7 @@ module Homebrew
           ).returns(T::Array[String])
         }
         def self.versions_from_content(content, regex = nil, &block)
-          items = items_from_content(content).sort_by { |item| [item.pub_date, item.bundle_version] }.reverse
+          items = sort_items(filter_items(items_from_content(content)))
           return [] if items.blank?
 
           item = items.first
diff --git a/Library/Homebrew/test/livecheck/strategy/sparkle_spec.rb b/Library/Homebrew/test/livecheck/strategy/sparkle_spec.rb
index 7eaaff5288..86a890151b 100644
--- a/Library/Homebrew/test/livecheck/strategy/sparkle_spec.rb
+++ b/Library/Homebrew/test/livecheck/strategy/sparkle_spec.rb
@@ -28,6 +28,17 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
   # `Sparkle::Item` objects.
   let(:item_hashes) do
     {
+      # The 1.2.4 version is only used in tests as the basis for an item that
+      # should be excluded (after modifications).
+      v124: {
+        title:                  "Version 1.2.4",
+        release_notes_link:     "https://www.example.com/example/1.2.4.html",
+        pub_date:               "Fri, 02 Jan 2021 01:23:45 +0000",
+        url:                    "https://www.example.com/example/example-1.2.4.tar.gz",
+        short_version:          "1.2.4",
+        version:                "124",
+        minimum_system_version: "10.10",
+      },
       v123: {
         title:                  "Version 1.2.3",
         release_notes_link:     "https://www.example.com/example/1.2.3.html",
@@ -128,6 +139,16 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
       </item>
     EOS
 
+    # Set the first item in a copy of `appcast` to a bad `minimumSystemVersion`
+    # value, to test `MacOSVersion::Error` handling.
+    bad_macos_version = appcast.sub(
+      v123_item,
+      v123_item.sub(
+        /(<sparkle:minimumSystemVersion>)[^<]+?</m,
+        '\1Not a macOS version<',
+      ),
+    )
+
     # Set the first item in a copy of `appcast` to the "beta" channel, to test
     # filtering items by channel using a `strategy` block.
     beta_channel_item = appcast.sub(
@@ -155,6 +176,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
     {
       appcast:             appcast,
       omitted_items:       omitted_items,
+      bad_macos_version:   bad_macos_version,
       beta_channel_item:   beta_channel_item,
       no_versions_item:    no_versions_item,
       no_items:            no_items,
@@ -166,6 +188,17 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
 
   let(:items) do
     {
+      v124: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
+        title:                  item_hashes[:v124][:title],
+        release_notes_link:     item_hashes[:v124][:release_notes_link],
+        pub_date:               Time.parse(item_hashes[:v124][:pub_date]),
+        url:                    item_hashes[:v124][:url],
+        bundle_version:         Homebrew::BundleVersion.new(
+          item_hashes[:v124][:short_version],
+          item_hashes[:v124][:version],
+        ),
+        minimum_system_version: MacOSVersion.new(item_hashes[:v124][:minimum_system_version]),
+      ),
       v123: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v123][:title],
         release_notes_link:     item_hashes[:v123][:release_notes_link],
@@ -175,7 +208,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v123][:short_version],
           item_hashes[:v123][:version],
         ),
-        minimum_system_version: item_hashes[:v123][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v123][:minimum_system_version]),
       ),
       v122: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v122][:title],
@@ -189,7 +222,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v122][:short_version],
           item_hashes[:v122][:version],
         ),
-        minimum_system_version: item_hashes[:v122][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v122][:minimum_system_version]),
       ),
       v121: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v121][:title],
@@ -201,7 +234,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v121][:short_version],
           item_hashes[:v121][:version],
         ),
-        minimum_system_version: item_hashes[:v121][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v121][:minimum_system_version]),
       ),
       v120: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v120][:title],
@@ -213,7 +246,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v120][:short_version],
           item_hashes[:v120][:version],
         ),
-        minimum_system_version: item_hashes[:v120][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v120][:minimum_system_version]),
       ),
     }
   end
@@ -234,6 +267,15 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
       ],
     }
 
+    bad_macos_version_item = items[:v123].clone
+    bad_macos_version_item.minimum_system_version = nil
+    item_arrays[:bad_macos_version] = [
+      bad_macos_version_item,
+      items[:v122],
+      items[:v121],
+      items[:v120],
+    ]
+
     beta_channel_item = items[:v123].clone
     beta_channel_item.channel = "beta"
     item_arrays[:beta_channel_item] = [
@@ -278,11 +320,40 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
       expect(items_from_appcast[0].short_version).to eq(item_hashes[:v123][:short_version])
       expect(items_from_appcast[0].version).to eq(item_hashes[:v123][:version])
 
+      expect(sparkle.items_from_content(xml[:bad_macos_version])).to eq(item_arrays[:bad_macos_version])
       expect(sparkle.items_from_content(xml[:beta_channel_item])).to eq(item_arrays[:beta_channel_item])
       expect(sparkle.items_from_content(xml[:no_versions_item])).to eq(item_arrays[:no_versions_item])
     end
   end
 
+  describe "::filter_items" do
+    let(:items_non_mac_os) do
+      item = items[:v124].clone
+      item.os = "not-osx-or-macos"
+      item_arrays[:appcast] + [item]
+    end
+
+    let(:items_prerelease_minimum_system_version) do
+      item = items[:v124].clone
+      item.minimum_system_version = MacOSVersion.new("100")
+      item_arrays[:appcast] + [item]
+    end
+
+    it "removes items with a non-mac OS" do
+      expect(sparkle.filter_items(items_non_mac_os)).to eq(item_arrays[:appcast])
+    end
+
+    it "removes items with a prerelease minimumSystemVersion" do
+      expect(sparkle.filter_items(items_prerelease_minimum_system_version)).to eq(item_arrays[:appcast])
+    end
+  end
+
+  describe "::sort_items" do
+    it "returns a sorted array of items" do
+      expect(sparkle.sort_items(item_arrays[:appcast])).to eq(item_arrays[:appcast_sorted])
+    end
+  end
+
   # `#versions_from_content` sorts items by `pub_date` and `bundle_version`, so
   # these tests have to account for this behavior in the expected output.
   # For example, the version 122 item doesn't have a parseable `pub_date` and

From d376b46a7ddfae55777675c4b037e7e6d23c0d9f Mon Sep 17 00:00:00 2001
From: Sam Ford <1584702+samford@users.noreply.github.com>
Date: Wed, 8 Nov 2023 09:13:09 -0500
Subject: [PATCH 3/6] Skip items strategy blocks in #livecheck_min_os

The `#livecheck_min_os` cask audit method should be skipped when a
`Sparkle` `livecheck` block contains a `strategy` block that uses
the `items` argument (instead of `item`). These `strategy` blocks
contain arbitrary logic that ignores/overrides the strategy's sorting,
so we can't identify which item would be first/newest.
---
 Library/Homebrew/cask/audit.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb
index 74f255020d..51b07e781b 100644
--- a/Library/Homebrew/cask/audit.rb
+++ b/Library/Homebrew/cask/audit.rb
@@ -597,6 +597,12 @@ module Cask
       return unless cask.livecheckable?
       return if cask.livecheck.strategy != :sparkle
 
+      # `Sparkle` strategy blocks that use the `items` argument (instead of
+      # `item`) contain arbitrary logic that ignores/overrides the strategy's
+      # sorting, so we can't identify which item would be first/newest here.
+      return if cask.livecheck.strategy_block.present? &&
+                cask.livecheck.strategy_block.parameters[0] == [:opt, :items]
+
       out, _, status = curl_output("--fail", "--silent", "--location", cask.livecheck.url)
       return unless status.success?
 

From 28451bd2bc88c8cedc3b0cd0dcec657c739c7e0e Mon Sep 17 00:00:00 2001
From: Sam Ford <1584702+samford@users.noreply.github.com>
Date: Wed, 8 Nov 2023 09:29:10 -0500
Subject: [PATCH 4/6] Use Sparkle sorting/filtering in #livecheck_min_os

The `#livecheck_min_os` cask audit method manually replicates some of
the `Sparkle` strategy's behavior but in an incomplete way that has
lead to inappropriate audit failures at times. This reimplements it
to use `Livecheck` methods, so it will align with the `Sparkle`
strategy's behavior.
---
 Library/Homebrew/cask/audit.rb | 38 ++++++++++++++--------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb
index 51b07e781b..1cc4433c19 100644
--- a/Library/Homebrew/cask/audit.rb
+++ b/Library/Homebrew/cask/audit.rb
@@ -603,31 +603,25 @@ module Cask
       return if cask.livecheck.strategy_block.present? &&
                 cask.livecheck.strategy_block.parameters[0] == [:opt, :items]
 
-      out, _, status = curl_output("--fail", "--silent", "--location", cask.livecheck.url)
-      return unless status.success?
-
-      require "rexml/document"
-
-      xml = begin
-        REXML::Document.new(out)
-      rescue REXML::ParseException
-        nil
-      end
-
-      return if xml.blank?
-
-      item = xml.elements["//rss//channel//item"]
-      return if item.blank?
-
-      min_os = item.elements["sparkle:minimumSystemVersion"]&.text
-      min_os = "11" if min_os == "10.16"
-      return if min_os.blank?
+      content = Homebrew::Livecheck::Strategy.page_content(cask.livecheck.url)[:content]
+      return if content.blank?
 
       begin
-        MacOSVersion.new(min_os).strip_patch
-      rescue MacOSVersion::Error
-        nil
+        items = Homebrew::Livecheck::Strategy::Sparkle.sort_items(
+          Homebrew::Livecheck::Strategy::Sparkle.filter_items(
+            Homebrew::Livecheck::Strategy::Sparkle.items_from_content(content),
+          ),
+        )
+      rescue
+        return
       end
+      return if items.blank?
+
+      min_os = items[0]&.minimum_system_version&.strip_patch
+      # Big Sur is sometimes identified as 10.16, so we override it to the
+      # expected macOS version (11).
+      min_os = MacOSVersion.new("11") if min_os == "10.16"
+      min_os
     end
 
     sig { returns(T.nilable(MacOSVersion)) }

From 491e2c4a80bf3add8994aef69e1bea8ed02d761a Mon Sep 17 00:00:00 2001
From: Sam Ford <1584702+samford@users.noreply.github.com>
Date: Wed, 8 Nov 2023 15:21:15 -0500
Subject: [PATCH 5/6] Reorder #audit_min_os array for consistency

This change doesn't affect the behavior of the `#audit_min_os`
method and simply reorders the array members to place `plist_min_os`
before `sparkle_min_os` for the sake of consistency (using the same
order as the preceding lines).
---
 Library/Homebrew/cask/audit.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb
index 1cc4433c19..e94d140d43 100644
--- a/Library/Homebrew/cask/audit.rb
+++ b/Library/Homebrew/cask/audit.rb
@@ -574,7 +574,7 @@ module Cask
       debug_messages << "Plist #{plist_min_os}" if plist_min_os
       debug_messages << "Sparkle #{sparkle_min_os}" if sparkle_min_os
       odebug "Minimum OS version: #{debug_messages.join(" | ")}" unless debug_messages.empty?
-      min_os = [sparkle_min_os, plist_min_os].compact.max
+      min_os = [plist_min_os, sparkle_min_os].compact.max
 
       return if min_os.nil? || min_os <= HOMEBREW_MACOS_OLDEST_ALLOWED
 

From 75ce7240fc7a0e815906795ed91dc3426b151fc2 Mon Sep 17 00:00:00 2001
From: Sam Ford <1584702+samford@users.noreply.github.com>
Date: Thu, 16 Nov 2023 11:25:18 -0500
Subject: [PATCH 6/6] Sparkle: Refactor macOS os strings into constant

---
 Library/Homebrew/livecheck/strategy/sparkle.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Library/Homebrew/livecheck/strategy/sparkle.rb b/Library/Homebrew/livecheck/strategy/sparkle.rb
index 1d6b32cf40..b7dc1b7cf8 100644
--- a/Library/Homebrew/livecheck/strategy/sparkle.rb
+++ b/Library/Homebrew/livecheck/strategy/sparkle.rb
@@ -21,6 +21,9 @@ module Homebrew
         # The `Regexp` used to determine if the strategy applies to the URL.
         URL_MATCH_REGEX = %r{^https?://}i.freeze
 
+        # Common `os` values used in appcasts to refer to macOS.
+        APPCAST_MACOS_STRINGS = ["macos", "osx"].freeze
+
         # Whether the strategy can be applied to the provided URL.
         #
         # @param url [String] the URL to match against
@@ -152,7 +155,7 @@ module Homebrew
         def self.filter_items(items)
           items.select do |item|
             # Omit items with an explicit `os` value that isn't macOS
-            next false if item.os && !((item.os == "osx") || (item.os == "macos"))
+            next false if item.os && APPCAST_MACOS_STRINGS.none?(item.os)
 
             # Omit items for prerelease macOS versions
             next false if item.minimum_system_version&.strip_patch&.prerelease?