diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index cfccde511a..18aec62629 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -193,13 +193,20 @@ class AbstractFileDownloadStrategy < AbstractDownloadStrategy private def ext + uri_path = if URI::DEFAULT_PARSER.make_regexp =~ @url + uri = URI(@url) + uri.query ? "#{uri.path}?#{uri.query}" : uri.path + else + @url + end + # We need a Pathname because we've monkeypatched extname to support double # extensions (e.g. tar.gz). # We can't use basename_without_params, because given a URL like # https://example.com/download.php?file=foo-1.0.tar.gz # the extension we want is ".tar.gz", not ".php". - Pathname.new(@url).ascend do |path| - ext = path.extname[/[^?]+/] + Pathname.new(uri_path).ascend do |path| + ext = path.extname[/[^?&]+/] return ext if ext end nil @@ -335,14 +342,15 @@ end # Query parameters on the URL are converted into POST parameters class CurlPostDownloadStrategy < CurlDownloadStrategy def _fetch - base_url, data = if meta.key?(:data) + args = if meta.key?(:data) escape_data = ->(d) { ["-d", URI.encode_www_form([d])] } - [@url, meta[:data].flat_map(&escape_data)] + [@url, *meta[:data].flat_map(&escape_data)] else - @url.split("?", 2) + url, query = @url.split("?", 2) + query.nil? ? [url, "-X", "POST"] : [url, "-d", query] end - curl_download base_url, "--data", data, to: temporary_path + curl_download(*args, to: temporary_path) end end @@ -606,9 +614,9 @@ class SubversionDownloadStrategy < VCSDownloadStrategy end if target.directory? - system_command("svn", args: ["update", *args], chdir: target.to_s) + system_command!("svn", args: ["update", *args], chdir: target.to_s) else - system_command("svn", args: ["checkout", url, target, *args]) + system_command!("svn", args: ["checkout", url, target, *args]) end end diff --git a/Library/Homebrew/test/download_strategies_spec.rb b/Library/Homebrew/test/download_strategies_spec.rb index 8c1a887e1e..b6e9e34d66 100644 --- a/Library/Homebrew/test/download_strategies_spec.rb +++ b/Library/Homebrew/test/download_strategies_spec.rb @@ -227,7 +227,7 @@ describe CurlDownloadStrategy do let(:name) { "foo" } let(:url) { "http://example.com/foo.tar.gz" } - let(:version) { nil } + let(:version) { "1.2.3" } let(:specs) { { user: "download:123456" } } it "parses the opts and sets the corresponding args" do @@ -238,13 +238,182 @@ describe CurlDownloadStrategy do subject { described_class.new(url, name, version, **specs).cached_location } context "when URL ends with file" do - it { is_expected.to eq(HOMEBREW_CACHE/"foo--.tar.gz") } + it { is_expected.to eq(HOMEBREW_CACHE/"foo--1.2.3.tar.gz") } end context "when URL file is in middle" do let(:url) { "http://example.com/foo.tar.gz/from/this/mirror" } - it { is_expected.to eq(HOMEBREW_CACHE/"foo--.tar.gz") } + it { is_expected.to eq(HOMEBREW_CACHE/"foo--1.2.3.tar.gz") } + end + end + + describe "#fetch" do + before(:each) do + FileUtils.touch subject.temporary_path + end + + it "calls curl with default arguments" do + expect(subject).to receive(:curl).with( + "--location", + "--remote-time", + "--continue-at", "-", + "--output", an_instance_of(Pathname), + url, + an_instance_of(Hash) + ) + + subject.fetch + end + + context "with an explicit user agent" do + let(:specs) { { user_agent: "Mozilla/25.0.1" } } + + it "adds the appropriate curl args" do + expect(subject).to receive(:system_command!) { |*, args:, **| + expect(args.each_cons(2)).to include(["--user-agent", "Mozilla/25.0.1"]) + } + + subject.fetch + end + end + + context "with a generalized fake user agent" do + alias_matcher :a_string_matching, :match + + let(:specs) { { user_agent: :fake } } + + it "adds the appropriate curl args" do + expect(subject).to receive(:system_command!) { |*, args:, **| + expect(args.each_cons(2).to_a).to include(["--user-agent", a_string_matching(/Mozilla.*Mac OS X 10.*AppleWebKit/)]) + } + + subject.fetch + end + end + + context "with cookies set" do + let(:specs) { + { + cookies: { + coo: "kie", + mon: "ster", + }, + } + } + + it "adds the appropriate curl args" do + expect(subject).to receive(:system_command!) { |*, args:, **| + expect(args.each_cons(2)).to include(["-b", "coo=kie;mon=ster"]) + } + + subject.fetch + end + end + + context "with referer set" do + let(:specs) { { referer: "http://somehost/also" } } + + it "adds the appropriate curl args" do + expect(subject).to receive(:system_command!) { |*, args:, **| + expect(args.each_cons(2)).to include(["-e", "http://somehost/also"]) + } + + subject.fetch + end + end + end + + describe "#cached_location" do + context "with a file name trailing the URL path" do + let(:url) { "http://example.com/cask.dmg" } + its("cached_location.extname") { is_expected.to eq(".dmg") } + end + + context "with no discernible file name in it" do + let(:url) { "http://example.com/download" } + its("cached_location.basename.to_path") { is_expected.to eq("foo--1.2.3") } + end + + context "with a file name trailing the first query parameter" do + let(:url) { "http://example.com/download?file=cask.zip&a=1" } + its("cached_location.extname") { is_expected.to eq(".zip") } + end + + context "with a file name trailing the second query parameter" do + let(:url) { "http://example.com/dl?a=1&file=cask.zip&b=2" } + its("cached_location.extname") { is_expected.to eq(".zip") } + end + + context "with an unusually long query string" do + let(:url) do + [ + "https://node49152.ssl.fancycdn.example.com", + "/fancycdn/node/49152/file/upload/download", + "?cask_class=zf920df", + "&cask_group=2348779087242312", + "&cask_archive_file_name=cask.zip", + "&signature=CGmDulxL8pmutKTlCleNTUY%2FyO9Xyl5u9yVZUE0", + "uWrjadjuz67Jp7zx3H7NEOhSyOhu8nzicEHRBjr3uSoOJzwkLC8L", + "BLKnz%2B2X%2Biq5m6IdwSVFcLp2Q1Hr2kR7ETn3rF1DIq5o0lHC", + "yzMmyNe5giEKJNW8WF0KXriULhzLTWLSA3ZTLCIofAdRiiGje1kN", + "YY3C0SBqymQB8CG3ONn5kj7CIGbxrDOq5xI2ZSJdIyPysSX7SLvE", + "DBw2KdR24q9t1wfjS9LUzelf5TWk6ojj8p9%2FHjl%2Fi%2FVCXN", + "N4o1mW%2FMayy2tTY1qcC%2FTmqI1ulZS8SNuaSgr9Iys9oDF1%2", + "BPK%2B4Sg==", + ].join + end + + its("cached_location.extname") { is_expected.to eq(".zip") } + its("cached_location.to_path.length") { is_expected.to be_between(0, 255) } + end + end +end + +describe CurlPostDownloadStrategy do + subject { described_class.new(url, name, version, **specs) } + + let(:name) { "foo" } + let(:url) { "http://example.com/foo.tar.gz" } + let(:version) { "1.2.3" } + let(:specs) { {} } + + describe "#fetch" do + before(:each) do + FileUtils.touch subject.temporary_path + end + + context "with :using and :data specified" do + let(:specs) { + { + using: :post, + data: { + form: "data", + is: "good", + }, + } + } + + it "adds the appropriate curl args" do + expect(subject).to receive(:system_command!) { |*, args:, **| + expect(args.each_cons(2)).to include(["-d", "form=data"]) + expect(args.each_cons(2)).to include(["-d", "is=good"]) + } + + subject.fetch + end + end + + context "with :using but no :data" do + let(:specs) { { using: :post } } + + it "adds the appropriate curl args" do + expect(subject).to receive(:system_command!) { |*, args:, **| + expect(args.each_cons(2)).to include(["-X", "POST"]) + } + + subject.fetch + end end end end @@ -329,6 +498,40 @@ describe ScpDownloadStrategy do end end +describe SubversionDownloadStrategy do + subject { described_class.new(url, name, version, **specs) } + + let(:name) { "foo" } + let(:url) { "http://example.com/foo.tar.gz" } + let(:version) { "1.2.3" } + let(:specs) { {} } + + describe "#fetch" do + context "with :trust_cert set" do + let(:specs) { { trust_cert: true } } + + it "adds the appropriate svn args" do + expect(subject).to receive(:system_command!) + .with("svn", args: array_including("--trust-server-cert", "--non-interactive")) + + subject.fetch + end + end + + context "with :revision set" do + let(:specs) { { revision: "10" } } + + it "adds svn arguments for :revision" do + expect(subject).to receive(:system_command!) { |*, args:, **| + expect(args.each_cons(2)).to include(["-r", "10"]) + } + + subject.fetch + end + end + end +end + describe DownloadStrategyDetector do describe "::detect" do subject { described_class.detect(url, strategy) }