Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rubocops/cask: Ensure that "verified" URLs with paths end with "/" #15125

Merged
merged 6 commits into from Apr 4, 2023
Merged

rubocops/cask: Ensure that "verified" URLs with paths end with "/" #15125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b586d97
rubocops/cask: Ensure that "verified" URLs with paths end with "/"
issyl0 Apr 2, 2023
17c0eaa
Fix indentation of `verified` in `url` stanza examples
issyl0 Apr 2, 2023
21da074
Only 'verified' stanzas with 0 or >1 path components should end with "/"
issyl0 Apr 2, 2023
41c3598
Simplify the 'should not start with https?://' message wording
issyl0 Apr 2, 2023
28f8cbe
Handle when the URL has interpolation: use `source` not `str_content`
issyl0 Apr 2, 2023
7bb20a3
Skip if the URL stanza has only two path components
issyl0 Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 23 additions & 7 deletions Library/Homebrew/rubocops/cask/url.rb
View file
Open in desktop
Expand Up @@ -8,13 +8,13 @@ module Cask
#
# @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
Expand All @@ -24,19 +24,35 @@ class Url < Base
def on_url_stanza(stanza)
return if stanza.stanza_node.block_type?

url_stanza = stanza.stanza_node.first_argument
hash_node = stanza.stanza_node.last_argument
return unless hash_node.hash_type?

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 contain a URL scheme.",
) do |corrector|
corrector.replace(value_node.source_range, value_node.source.gsub(%r{^"https?://}, "\""))
end
end

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@issyl0, this only fixes this particular case.

What if this isn't a GitHub URL with this specific format?

url "https://example.org/long/path/app-1.2.3.dmg",
    verified: "example.org/long/path/app"

Granted I don't think we currently have any cask with such a format, but 2 is kinda arbitrary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are overcomplicating the condition for this cop.

If verified doesn't already end with a slash but the url starts with "#{verified}/", a slash should be added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reitermarkus That sounds good, could you open a PR for that?

# Skip if the verified value ends with a slash.
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
Expand Down
194 changes: 188 additions & 6 deletions Library/Homebrew/test/rubocops/cask/url_spec.rb
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@
<<~CASK
cask "foo" do
url "https://example.com/download/foo-v1.2.0.dmg",
verified: "example.com"
verified: "example.com/download/"
end
CASK
end
Expand All @@ -27,26 +27,208 @@
<<~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

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: 14,
source: "\"https://example.com\"",
column: 16,
source: "\"https://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"
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
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

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

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://github.com/Foo/foo/releases/download/v1.2.0/foo-v1.2.0.dmg",
verified: "github.com/Foo/foo/"
end
CASK
end

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
cask "foo" do
url "https://github.com/Foo/foo/releases/download/v1.2.0/foo-v1.2.0.dmg",
verified: "github.com/Foo/foo"
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/Foo/foo\"",
}]
end

let(:correct_source) do
<<~CASK
cask "foo" do
url "https://github.com/Foo/foo/releases/download/v1.2.0/foo-v1.2.0.dmg",
verified: "github.com/Foo/foo/"
end
CASK
end
Expand Down