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.

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

Curl: Use typed: strict #19077

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Library/Homebrew/cask/cask_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ def load(config:)

begin
ohai "Downloading #{url}"
::Utils::Curl.curl_download url, to: path
::Utils::Curl.curl_download url.to_s, to: path
rescue ErrorDuringExecution
raise CaskUnavailableError.new(token, "Failed to download #{Formatter.url(url)}.")
end
Expand Down
18 changes: 10 additions & 8 deletions Library/Homebrew/cask/url.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,21 +99,26 @@

class BlockDSL
# Allow accessing the URL associated with page contents.
module PageWithURL
class PageWithURL < SimpleDelegator
# Get the URL of the fetched page.
#
# ### Example
#
# ```ruby
# url "https://example.org/download" do |page|
# file = page[/href="([^"]+.dmg)"/, 1]
# URL.join(page.url, file)
# file_path = page[/href="([^"]+\.dmg)"/, 1]
# URI.join(page.url, file_path)
# end
# ```
#
# @api public
sig { returns(URI::Generic) }
attr_accessor :url

def initialize(str, url)
super(str)
@url = url

Check warning on line 120 in Library/Homebrew/cask/url.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/url.rb#L119-L120

Added lines #L119 - L120 were not covered by tests
end
end

sig {
Expand All @@ -135,13 +140,10 @@
sig { returns(T.any(T.any(URI::Generic, String), [T.any(URI::Generic, String), Hash])) }
def call
if @uri
result = ::Utils::Curl.curl_output("--fail", "--silent", "--location", @uri)
result = ::Utils::Curl.curl_output("--fail", "--silent", "--location", @uri.to_s)
result.assert_success!

page = result.stdout
page.extend PageWithURL
page.url = URI(@uri)

page = PageWithURL.new(result.stdout, URI(@uri))

Check warning on line 146 in Library/Homebrew/cask/url.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/url.rb#L146

Added line #L146 was not covered by tests
instance_exec(page, &@block)
else
instance_exec(&@block)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/formulary.rb
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,7 @@ def load_file(flags:, ignore_errors:)
end
HOMEBREW_CACHE_FORMULA.mkpath
FileUtils.rm_f(path)
Utils::Curl.curl_download url, to: path
Utils::Curl.curl_download url.to_s, to: path
super
rescue MethodDeprecatedError => e
if (match_data = url.match(%r{github.com/(?<user>[\w-]+)/(?<repo>[\w-]+)/}).presence)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/github_packages.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@
# Going forward, this should probably be pinned to tags.
# We currently use features newer than the last one (v1.0.2).
url = "https://raw.githubusercontent.com/opencontainers/image-spec/170393e57ed656f7f81c3070bfa8c3346eaa0a5a/schema/#{basename}.json"
out, = Utils::Curl.curl_output(url)
out = Utils::Curl.curl_output(url).stdout

Check warning on line 172 in Library/Homebrew/github_packages.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/github_packages.rb#L172

Added line #L172 was not covered by tests
json = JSON.parse(out)

@schema_json ||= {}
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/test/download_strategies/curl_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
it "calls curl with default arguments" do
expect(strategy).to receive(:curl).with(
"--remote-time",
"--output", an_instance_of(Pathname),
"--output", an_instance_of(String),
# example.com supports partial requests.
"--continue-at", "-",
"--location",
Expand Down
188 changes: 181 additions & 7 deletions Library/Homebrew/test/utils/curl_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@
},
}

response_hash[:ok_no_status_text] = response_hash[:ok].deep_dup
response_hash[:ok_no_status_text].delete(:status_text)

response_hash[:ok_blank_header_value] = response_hash[:ok].deep_dup
response_hash[:ok_blank_header_value][:headers]["range"] = ""

response_hash[:redirection] = {
status_code: "301",
status_text: "Moved Permanently",
Expand Down Expand Up @@ -257,6 +263,16 @@
\r
EOS

response_text[:ok_no_status_text] = response_text[:ok].sub(" #{response_hash[:ok][:status_text]}", "")
response_text[:ok_blank_header_name] = response_text[:ok].sub(
"#{response_hash[:ok][:headers]["date"]}\r\n",
"#{response_hash[:ok][:headers]["date"]}\r\n: Test\r\n",
)
response_text[:ok_blank_header_value] = response_text[:ok].sub(
"#{response_hash[:ok][:headers]["date"]}\r\n",
"#{response_hash[:ok][:headers]["date"]}\r\nRange:\r\n",
)

response_text[:redirection] = response_text[:ok].sub(
"HTTP/1.1 #{response_hash[:ok][:status_code]} #{response_hash[:ok][:status_text]}\r",
"HTTP/1.1 #{response_hash[:redirection][:status_code]} #{response_hash[:redirection][:status_text]}\r\n" \
Expand Down Expand Up @@ -306,7 +322,27 @@
body
end

describe "curl_args" do
describe "::curl_executable" do
it "returns `HOMEBREW_BREWED_CURL_PATH` when `use_homebrew_curl` is `true`" do
expect(curl_executable(use_homebrew_curl: true)).to eq(HOMEBREW_BREWED_CURL_PATH)
end

it "returns curl shim path when `use_homebrew_curl` is `false` or omitted" do
curl_shim_path = HOMEBREW_SHIMS_PATH/"shared/curl"
expect(curl_executable(use_homebrew_curl: false)).to eq(curl_shim_path)
expect(curl_executable).to eq(curl_shim_path)
end
end

describe "::curl_path" do
it "returns a curl path string" do
expect(curl_path).to match(%r{[^/]+(?:/[^/]+)*})
end
end

describe "::curl_args" do
include Context

let(:args) { ["foo"] }
let(:user_agent_string) { "Lorem ipsum dolor sit amet" }

Expand Down Expand Up @@ -388,6 +424,11 @@
expect { curl_args(*args, retry_max_time: "test") }.to raise_error(TypeError)
end

it "uses `--show-error` when :show_error is `true`" do
expect(curl_args(*args, show_error: true)).to include("--show-error")
expect(curl_args(*args, show_error: false)).not_to include("--show-error")
end

it "uses `--referer` when :referer is present" do
expect(curl_args(*args, referer: "https://brew.sh").join(" ")).to include("--referer https://brew.sh")
end
Expand Down Expand Up @@ -426,9 +467,62 @@
expect(curl_args(*args).join(" ")).to include("--fail")
expect(curl_args(*args, show_output: true).join(" ")).not_to include("--fail")
end

it "uses `--progress-bar` outside of a `--verbose` context" do
expect(curl_args(*args).join(" ")).to include("--progress-bar")
with_context verbose: true do
expect(curl_args(*args).join(" ")).not_to include("--progress-bar")
end
end

context "when `EnvConfig.curl_verbose?` is `true`" do
before do
allow(Homebrew::EnvConfig).to receive(:curl_verbose?).and_return(true)
end

it "uses `--verbose`" do
expect(curl_args(*args).join(" ")).to include("--verbose")
end
end

context "when `EnvConfig.curl_verbose?` is `false`" do
before do
allow(Homebrew::EnvConfig).to receive(:curl_verbose?).and_return(false)
end

it "doesn't use `--verbose`" do
expect(curl_args(*args).join(" ")).not_to include("--verbose")
end
end

context "when `$stdout.tty?` is `false`" do
before do
allow($stdout).to receive(:tty?).and_return(false)
end

it "uses `--silent`" do
expect(curl_args(*args).join(" ")).to include("--silent")
end
end

context "when `$stdout.tty?` is `true`" do
before do
allow($stdout).to receive(:tty?).and_return(true)
end

it "doesn't use `--silent` outside of a `--quiet` context" do
with_context quiet: false do
expect(curl_args(*args).join(" ")).not_to include("--silent")
end

with_context quiet: true do
expect(curl_args(*args).join(" ")).to include("--silent")
end
end
end
end

describe "url_protected_by_cloudflare?" do
describe "::url_protected_by_cloudflare?" do
it "returns `true` when a URL is protected by Cloudflare" do
expect(url_protected_by_cloudflare?(details[:cloudflare][:single_cookie])).to be(true)
expect(url_protected_by_cloudflare?(details[:cloudflare][:multiple_cookies])).to be(true)
Expand All @@ -448,7 +542,7 @@
end
end

describe "url_protected_by_incapsula?" do
describe "::url_protected_by_incapsula?" do
it "returns `true` when a URL is protected by Cloudflare" do
expect(url_protected_by_incapsula?(details[:incapsula][:single_cookie_visid_incap])).to be(true)
expect(url_protected_by_incapsula?(details[:incapsula][:single_cookie_incap_ses])).to be(true)
Expand All @@ -468,7 +562,54 @@
end
end

describe "#parse_curl_output" do
describe "::curl_version" do
it "returns a curl version string" do
expect(curl_version).to match(/^v?(\d+(?:\.\d+)+)$/)
end
end

describe "::curl_supports_fail_with_body?" do
it "returns `true` if curl version is 7.76.0 or higher" do
allow_any_instance_of(Utils::Curl).to receive(:curl_version).and_return(Version.new("7.76.0"))
expect(curl_supports_fail_with_body?).to be(true)

allow_any_instance_of(Utils::Curl).to receive(:curl_version).and_return(Version.new("7.76.1"))
expect(curl_supports_fail_with_body?).to be(true)
end

it "returns `false` if curl version is lower than 7.76.0" do
allow_any_instance_of(Utils::Curl).to receive(:curl_version).and_return(Version.new("7.75.0"))
expect(curl_supports_fail_with_body?).to be(false)
end
end

describe "::curl_supports_tls13?" do
it "returns `true` if curl command is successful" do
allow_any_instance_of(Kernel).to receive(:quiet_system).and_return(true)
expect(curl_supports_tls13?).to be(true)
end

it "returns `false` if curl command is not successful" do
allow_any_instance_of(Kernel).to receive(:quiet_system).and_return(false)
expect(curl_supports_tls13?).to be(false)
end
end

describe "::http_status_ok?" do
it "returns `true` when `status` is 1xx or 2xx" do
expect(http_status_ok?("200")).to be(true)
end

it "returns `false` when `status` is not 1xx or 2xx" do
expect(http_status_ok?("301")).to be(false)
end

it "returns `false` when `status` is `nil`" do
expect(http_status_ok?(nil)).to be(false)
end
end

describe "::parse_curl_output" do
it "returns a correct hash when curl output contains response(s) and body" do
expect(parse_curl_output("#{response_text[:ok]}#{body[:default]}"))
.to eq({ responses: [response_hash[:ok]], body: body[:default] })
Expand Down Expand Up @@ -505,21 +646,33 @@
it "returns correct hash when curl output is blank" do
expect(parse_curl_output("")).to eq({ responses: [], body: "" })
end

it "errors if response count exceeds `max_iterations`" do
expect do
parse_curl_output(response_text[:redirections_to_ok], max_iterations: 1)
end.to raise_error("Too many redirects (max = 1)")
end
end

describe "#parse_curl_response" do
describe "::parse_curl_response" do
it "returns a correct hash when given HTTP response text" do
expect(parse_curl_response(response_text[:ok])).to eq(response_hash[:ok])
expect(parse_curl_response(response_text[:ok_no_status_text])).to eq(response_hash[:ok_no_status_text])
expect(parse_curl_response(response_text[:ok_blank_header_value])).to eq(response_hash[:ok_blank_header_value])
expect(parse_curl_response(response_text[:redirection])).to eq(response_hash[:redirection])
expect(parse_curl_response(response_text[:duplicate_header])).to eq(response_hash[:duplicate_header])
end

it "skips over response header lines with blank header name" do
expect(parse_curl_response(response_text[:ok_blank_header_name])).to eq(response_hash[:ok])
end

it "returns an empty hash when given an empty string" do
expect(parse_curl_response("")).to eq({})
end
end

describe "#curl_response_last_location" do
describe "::curl_response_last_location" do
it "returns the last location header when given an array of HTTP response hashes" do
expect(curl_response_last_location([
response_hash[:redirection],
Expand Down Expand Up @@ -577,12 +730,20 @@
).to eq(response_hash[:redirection_parent_relative][:headers]["location"].sub(/^\./, "https://brew.sh/test1"))
end

it "skips response hashes without a `:headers` value" do
expect(curl_response_last_location([
response_hash[:redirection],
{ status_code: "404", status_text: "Not Found" },
response_hash[:ok],
])).to eq(response_hash[:redirection][:headers]["location"])
end

it "returns nil when the response hash doesn't contain a location header" do
expect(curl_response_last_location([response_hash[:ok]])).to be_nil
end
end

describe "#curl_response_follow_redirections" do
describe "::curl_response_follow_redirections" do
it "returns the original URL when there are no location headers" do
expect(
curl_response_follow_redirections(
Expand Down Expand Up @@ -634,5 +795,18 @@
),
).to eq("#{location_urls[0]}example/")
end

it "skips response hashes without a `:headers` value" do
expect(
curl_response_follow_redirections(
[
response_hash[:redirection_root_relative],
{ status_code: "404", status_text: "Not Found" },
response_hash[:ok],
],
"https://brew.sh/test1/test2",
),
).to eq("https://brew.sh/example/")
end
end
end
6 changes: 3 additions & 3 deletions Library/Homebrew/utils/analytics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@
formula_version_urls = output.stdout
.scan(%r{/orgs/Homebrew/packages/#{formula_url_suffix}\d+\?tag=[^"]+})
.map do |url|
url.sub("/orgs/Homebrew/packages/", "/Homebrew/homebrew-core/pkgs/")
T.cast(url, String).sub("/orgs/Homebrew/packages/", "/Homebrew/homebrew-core/pkgs/")

Check warning on line 292 in Library/Homebrew/utils/analytics.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/analytics.rb#L292

Added line #L292 was not covered by tests
end
return if formula_version_urls.empty?

Expand All @@ -304,9 +304,9 @@
)
next if last_thirty_days_match.blank?

last_thirty_days_downloads = last_thirty_days_match.captures.first.tr(",", "")
last_thirty_days_downloads = T.must(last_thirty_days_match.captures.first).tr(",", "")

Check warning on line 307 in Library/Homebrew/utils/analytics.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/analytics.rb#L307

Added line #L307 was not covered by tests
thirty_day_download_count += if (millions_match = last_thirty_days_downloads.match(/(\d+\.\d+)M/).presence)
millions_match.captures.first.to_f * 1_000_000
(millions_match.captures.first.to_f * 1_000_000).to_i
else
last_thirty_days_downloads.to_i
end
Expand Down
Loading
Loading