From b586d97f84405498acd711fa97e4f34ce22b094a Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 2 Apr 2023 15:29:17 +0100 Subject: [PATCH 1/6] rubocops/cask: Ensure that "verified" URLs with paths end with "/" - These were being fixed manually[1], so let's make a RuboCop for any further occurrences since this is a good rule to enforce[2]. [1] - https://github.com/Homebrew/homebrew-cask/pull/144179#issuecomment-1489857249 [2] - https://github.com/Homebrew/homebrew-cask/pull/80965#issuecomment-616232313 --- Library/Homebrew/rubocops/cask/url.rb | 25 +++++++--- .../Homebrew/test/rubocops/cask/url_spec.rb | 46 +++++++++++++++++++ 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/url.rb b/Library/Homebrew/rubocops/cask/url.rb index b2fe767ac5..2793d29cf4 100644 --- a/Library/Homebrew/rubocops/cask/url.rb +++ b/Library/Homebrew/rubocops/cask/url.rb @@ -8,13 +8,13 @@ module RuboCop # # @example # # bad - # url "https://example.com/foo.dmg", - # verified: "https://example.com" + # url "https://example.com/download/foo.dmg", + # verified: "https://example.com/download" # # # # good - # url "https://example.com/foo.dmg", - # verified: "example.com" + # url "https://example.com/download/foo.dmg", + # verified: "example.com/download/" # class Url < Base extend AutoCorrector @@ -30,13 +30,24 @@ module RuboCop hash_node.each_pair do |key_node, value_node| next unless key_node.source == "verified" next unless value_node.str_type? - next unless value_node.source.start_with?(%r{^"https?://}) + + if value_node.source.start_with?(%r{^"https?://}) + add_offense( + value_node.source_range, + message: "Verified URL parameter value should not start with https:// or http://.", + ) do |corrector| + corrector.replace(value_node.source_range, value_node.source.gsub(%r{^"https?://}, "\"")) + end + end + + next unless value_node.str_content.gsub(%r{https?://}, "").include?("/") # Skip if the stanza has no path. + next if value_node.str_content.end_with?("/") add_offense( value_node.source_range, - message: "Verified URL parameter value should not start with https:// or http://.", + message: "Verified URL parameter value should end with a /.", ) do |corrector| - corrector.replace(value_node.source_range, value_node.source.gsub(%r{^"https?://}, "\"")) + corrector.replace(value_node.source_range, value_node.source.gsub(/"$/, "/\"")) end end end diff --git a/Library/Homebrew/test/rubocops/cask/url_spec.rb b/Library/Homebrew/test/rubocops/cask/url_spec.rb index 23ef640b60..92c182c894 100644 --- a/Library/Homebrew/test/rubocops/cask/url_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/url_spec.rb @@ -54,4 +54,50 @@ describe RuboCop::Cop::Cask::Url do include_examples "reports offenses" include_examples "autocorrects source" end + + context "when url 'verified' value has a path component that ends with a /" do + let(:source) do + <<~CASK + cask "foo" do + url "https://example.com/download/foo-v1.2.0.dmg", + verified: "example.com/download/" + end + CASK + end + + include_examples "does not report any offenses" + end + + context "when the url 'verified' value has a path component that doesn't end with a /" do + let(:source) do + <<~CASK + cask "foo" do + url "https://example.com/download/foo-v1.2.0.dmg", + verified: "example.com/download" + end + CASK + end + + let(:expected_offenses) do + [{ + message: "Verified URL parameter value should end with a /.", + severity: :convention, + line: 3, + column: 14, + source: "\"example.com/download\"", + }] + end + + let(:correct_source) do + <<~CASK + cask "foo" do + url "https://example.com/download/foo-v1.2.0.dmg", + verified: "example.com/download/" + end + CASK + end + + include_examples "reports offenses" + include_examples "autocorrects source" + end end From 17c0eaab25098075d55dfb49fd81ef5dcbf78f19 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 2 Apr 2023 16:38:33 +0100 Subject: [PATCH 2/6] Fix indentation of `verified` in `url` stanza examples --- Library/Homebrew/rubocops/cask/url.rb | 4 ++-- Library/Homebrew/test/rubocops/cask/url_spec.rb | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/url.rb b/Library/Homebrew/rubocops/cask/url.rb index 2793d29cf4..49b850edfb 100644 --- a/Library/Homebrew/rubocops/cask/url.rb +++ b/Library/Homebrew/rubocops/cask/url.rb @@ -9,12 +9,12 @@ module RuboCop # @example # # bad # url "https://example.com/download/foo.dmg", - # verified: "https://example.com/download" + # verified: "https://example.com/download" # # # # good # url "https://example.com/download/foo.dmg", - # verified: "example.com/download/" + # verified: "example.com/download/" # class Url < Base extend AutoCorrector diff --git a/Library/Homebrew/test/rubocops/cask/url_spec.rb b/Library/Homebrew/test/rubocops/cask/url_spec.rb index 92c182c894..daef684d95 100644 --- a/Library/Homebrew/test/rubocops/cask/url_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/url_spec.rb @@ -14,7 +14,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com" + verified: "example.com" end CASK end @@ -27,7 +27,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "https://example.com" + verified: "https://example.com" end CASK end @@ -37,7 +37,7 @@ describe RuboCop::Cop::Cask::Url do message: "Verified URL parameter value should not start with https:// or http://.", severity: :convention, line: 3, - column: 14, + column: 16, source: "\"https://example.com\"", }] end @@ -46,7 +46,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com" + verified: "example.com" end CASK end @@ -60,7 +60,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com/download/" + verified: "example.com/download/" end CASK end @@ -73,7 +73,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com/download" + verified: "example.com/download" end CASK end @@ -83,7 +83,7 @@ describe RuboCop::Cop::Cask::Url do message: "Verified URL parameter value should end with a /.", severity: :convention, line: 3, - column: 14, + column: 16, source: "\"example.com/download\"", }] end @@ -92,7 +92,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com/download/" + verified: "example.com/download/" end CASK end From 21da0743464e1d81f0768b266b888cda68fbe8c7 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 2 Apr 2023 19:39:23 +0100 Subject: [PATCH 3/6] Only 'verified' stanzas with 0 or >1 path components should end with "/" Handle good things like: ```ruby url "https://example.org/download", verified: "example.org/download" # This is fine. ``` And bad things like: ```ruby url "https://example.org/", verified: "example.org" # This should end with a slash. ``` --- Library/Homebrew/rubocops/cask/url.rb | 5 +- .../Homebrew/test/rubocops/cask/url_spec.rb | 129 ++++++++++++++++-- 2 files changed, 122 insertions(+), 12 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/url.rb b/Library/Homebrew/rubocops/cask/url.rb index 49b850edfb..a82546610e 100644 --- a/Library/Homebrew/rubocops/cask/url.rb +++ b/Library/Homebrew/rubocops/cask/url.rb @@ -24,6 +24,7 @@ module RuboCop def on_url_stanza(stanza) return if stanza.stanza_node.block_type? + url_string = stanza.stanza_node.first_argument.str_content hash_node = stanza.stanza_node.last_argument return unless hash_node.hash_type? @@ -40,7 +41,9 @@ module RuboCop end end - next unless value_node.str_content.gsub(%r{https?://}, "").include?("/") # Skip if the stanza has no path. + # Skip if the URL and the verified value are the same. + next if value_node.str_content == url_string.delete_prefix("https://").delete_prefix("http://") + # Skip if the verified value ends with a slash. next if value_node.str_content.end_with?("/") add_offense( diff --git a/Library/Homebrew/test/rubocops/cask/url_spec.rb b/Library/Homebrew/test/rubocops/cask/url_spec.rb index daef684d95..818e11f4f9 100644 --- a/Library/Homebrew/test/rubocops/cask/url_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/url_spec.rb @@ -14,7 +14,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com" + verified: "example.com/download/" end CASK end @@ -27,7 +27,7 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "https://example.com" + verified: "https://example.com/download/" end CASK end @@ -38,7 +38,7 @@ describe RuboCop::Cop::Cask::Url do severity: :convention, line: 3, column: 16, - source: "\"https://example.com\"", + source: "\"https://example.com/download/\"", }] end @@ -46,7 +46,114 @@ describe RuboCop::Cop::Cask::Url do <<~CASK cask "foo" do url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com" + verified: "example.com/download/" + end + CASK + end + + include_examples "reports offenses" + include_examples "autocorrects source" + end + + context "when url 'verified' value does not have a path component" do + context "when the URL ends with a slash" do + let(:source) do + <<~CASK + cask "foo" do + url "https://example.org/", + verified: "example.org/" + end + CASK + end + + include_examples "does not report any offenses" + end + + context "when the URL does not end with a slash" do + let(:source) do + <<~CASK + cask "foo" do + url "https://example.org/", + verified: "example.org" + end + CASK + end + + let(:expected_offenses) do + [{ + message: "Verified URL parameter value should end with a /.", + severity: :convention, + line: 3, + column: 16, + source: "\"example.org\"", + }] + end + + let(:correct_source) do + <<~CASK + cask "foo" do + url "https://example.org/", + verified: "example.org/" + end + CASK + end + + include_examples "reports offenses" + include_examples "autocorrects source" + end + end + + context "when the URL does not end with a slash" do + let(:source) do + <<~CASK + cask "foo" do + url "https://github.com/Foo", + verified: "github.com/Foo" + end + CASK + end + + include_examples "does not report any offenses" + end + + context "when the url ends with a / and the verified value does too" do + let(:source) do + <<~CASK + cask "foo" do + url "https://github.com/", + verified: "github.com/" + end + CASK + end + + include_examples "does not report any offenses" + end + + context "when the url ends with a / and the verified value does not" do + let(:source) do + <<~CASK + cask "foo" do + url "https://github.com/", + verified: "github.com" + end + CASK + end + + let(:expected_offenses) do + [{ + message: "Verified URL parameter value should end with a /.", + severity: :convention, + line: 3, + column: 16, + source: "\"github.com\"", + }] + end + + let(:correct_source) do + <<~CASK + cask "foo" do + url "https://github.com/", + verified: "github.com/" end CASK end @@ -59,8 +166,8 @@ describe RuboCop::Cop::Cask::Url do let(:source) do <<~CASK cask "foo" do - url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com/download/" + url "https://github.com/Foo/foo/releases/download/v1.2.0/foo-v1.2.0.dmg", + verified: "github.com/Foo/foo/" end CASK end @@ -72,8 +179,8 @@ describe RuboCop::Cop::Cask::Url do let(:source) do <<~CASK cask "foo" do - url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com/download" + url "https://github.com/Foo/foo/releases/download/v1.2.0/foo-v1.2.0.dmg", + verified: "github.com/Foo/foo" end CASK end @@ -84,15 +191,15 @@ describe RuboCop::Cop::Cask::Url do severity: :convention, line: 3, column: 16, - source: "\"example.com/download\"", + source: "\"github.com/Foo/foo\"", }] end let(:correct_source) do <<~CASK cask "foo" do - url "https://example.com/download/foo-v1.2.0.dmg", - verified: "example.com/download/" + url "https://github.com/Foo/foo/releases/download/v1.2.0/foo-v1.2.0.dmg", + verified: "github.com/Foo/foo/" end CASK end From 41c35986f819616a49f22ba7f33b74064bd54392 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 2 Apr 2023 22:43:52 +0100 Subject: [PATCH 4/6] Simplify the 'should not start with https?://' message wording Co-authored-by: Markus Reiter --- Library/Homebrew/rubocops/cask/url.rb | 2 +- Library/Homebrew/test/rubocops/cask/url_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/url.rb b/Library/Homebrew/rubocops/cask/url.rb index a82546610e..9ed9b85acb 100644 --- a/Library/Homebrew/rubocops/cask/url.rb +++ b/Library/Homebrew/rubocops/cask/url.rb @@ -35,7 +35,7 @@ module RuboCop if value_node.source.start_with?(%r{^"https?://}) add_offense( value_node.source_range, - message: "Verified URL parameter value should not start with https:// or http://.", + message: "Verified URL parameter value should not contain a URL scheme.", ) do |corrector| corrector.replace(value_node.source_range, value_node.source.gsub(%r{^"https?://}, "\"")) end diff --git a/Library/Homebrew/test/rubocops/cask/url_spec.rb b/Library/Homebrew/test/rubocops/cask/url_spec.rb index 818e11f4f9..93619d376b 100644 --- a/Library/Homebrew/test/rubocops/cask/url_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/url_spec.rb @@ -34,7 +34,7 @@ describe RuboCop::Cop::Cask::Url do let(:expected_offenses) do [{ - message: "Verified URL parameter value should not start with https:// or http://.", + message: "Verified URL parameter value should not contain a URL scheme.", severity: :convention, line: 3, column: 16, From 28f8cbe8daf8a8b96fef9df92fd6c63e0f342fc7 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 2 Apr 2023 23:00:14 +0100 Subject: [PATCH 5/6] Handle when the URL has interpolation: use `source` not `str_content` - We see this a lot in real Casks. --- Library/Homebrew/rubocops/cask/url.rb | 4 ++-- Library/Homebrew/test/rubocops/cask/url_spec.rb | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/url.rb b/Library/Homebrew/rubocops/cask/url.rb index 9ed9b85acb..ac20d2c275 100644 --- a/Library/Homebrew/rubocops/cask/url.rb +++ b/Library/Homebrew/rubocops/cask/url.rb @@ -24,7 +24,7 @@ module RuboCop def on_url_stanza(stanza) return if stanza.stanza_node.block_type? - url_string = stanza.stanza_node.first_argument.str_content + url_stanza = stanza.stanza_node.first_argument hash_node = stanza.stanza_node.last_argument return unless hash_node.hash_type? @@ -42,7 +42,7 @@ module RuboCop end # Skip if the URL and the verified value are the same. - next if value_node.str_content == url_string.delete_prefix("https://").delete_prefix("http://") + next if value_node.source == url_stanza.source.gsub(%r{^"https?://}, "\"") # Skip if the verified value ends with a slash. next if value_node.str_content.end_with?("/") diff --git a/Library/Homebrew/test/rubocops/cask/url_spec.rb b/Library/Homebrew/test/rubocops/cask/url_spec.rb index 93619d376b..1b768231dc 100644 --- a/Library/Homebrew/test/rubocops/cask/url_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/url_spec.rb @@ -175,6 +175,20 @@ describe RuboCop::Cop::Cask::Url do include_examples "does not report any offenses" end + context "when the url has interpolation in it and the verified url ends with a /" do + let(:source) do + <<~CASK + cask "foo" do + version "1.2.3" + url "https://example.com/download/foo-v\#{version}.dmg", + verified: "example.com/download/" + end + CASK + end + + include_examples "does not report any offenses" + end + context "when the url 'verified' value has a path component that doesn't end with a /" do let(:source) do <<~CASK From 7bb20a3b83ac4978c1e1d2b4a7c8d30f3e928860 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 4 Apr 2023 16:11:44 +0100 Subject: [PATCH 6/6] Skip if the URL stanza has only two path components https://github.com/Homebrew/homebrew-cask-fonts/pull/7336#discussion_r1156325651 --- Library/Homebrew/rubocops/cask/url.rb | 2 ++ .../Homebrew/test/rubocops/cask/url_spec.rb | 31 ++++++++++++++----- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/url.rb b/Library/Homebrew/rubocops/cask/url.rb index ac20d2c275..2d3882d938 100644 --- a/Library/Homebrew/rubocops/cask/url.rb +++ b/Library/Homebrew/rubocops/cask/url.rb @@ -43,6 +43,8 @@ module RuboCop # Skip if the URL and the verified value are the same. next if value_node.source == url_stanza.source.gsub(%r{^"https?://}, "\"") + # Skip if the URL has two path components, eg: `https://github.com/google/fonts.git`. + next if url_stanza.source.gsub(%r{^"https?://}, "\"").count("/") == 2 # Skip if the verified value ends with a slash. next if value_node.str_content.end_with?("/") diff --git a/Library/Homebrew/test/rubocops/cask/url_spec.rb b/Library/Homebrew/test/rubocops/cask/url_spec.rb index 1b768231dc..4698bd94a4 100644 --- a/Library/Homebrew/test/rubocops/cask/url_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/url_spec.rb @@ -104,16 +104,31 @@ describe RuboCop::Cop::Cask::Url do end context "when the URL does not end with a slash" do - let(:source) do - <<~CASK - cask "foo" do - url "https://github.com/Foo", - verified: "github.com/Foo" - end - CASK + describe "and it has one path component" do + let(:source) do + <<~CASK + cask "foo" do + url "https://github.com/Foo", + verified: "github.com/Foo" + end + CASK + end + + include_examples "does not report any offenses" end - include_examples "does not report any offenses" + describe "and it has two path components" do + let(:source) do + <<~CASK + cask "foo" do + url "https://github.com/foo/foo.git", + verified: "github.com/foo/foo" + end + CASK + end + + include_examples "does not report any offenses" + end end context "when the url ends with a / and the verified value does too" do