Merge pull request #16416 from Bo98/safe-filename

Add consistent path validation

Library/Homebrew/dev-cmd/bottle.rb

@@ -365,9 +365,9 @@ module Homebrew
       end || 0
     end
 
-    filename = Bottle::Filename.create(formula, bottle_tag.to_sym, rebuild)
+    filename = Bottle::Filename.create(formula, bottle_tag, rebuild)
     local_filename = filename.to_s
-    bottle_path = Pathname.pwd/filename
+    bottle_path = Pathname.pwd/local_filename
 
     tab = nil
     keg = nil
@@ -690,8 +690,8 @@ module Homebrew
       bottle_hash["bottle"]["tags"].each do |tag, tag_hash|
         filename = Bottle::Filename.new(
           formula_name,
-          bottle_hash["formula"]["pkg_version"],
-          tag,
+          PkgVersion.parse(bottle_hash["formula"]["pkg_version"]),
+          Utils::Bottles::Tag.from_symbol(tag.to_sym),
           bottle_hash["bottle"]["rebuild"],
         )
 
@@ -700,8 +700,8 @@ module Homebrew
 
           all_filename = Bottle::Filename.new(
             formula_name,
-            bottle_hash["formula"]["pkg_version"],
-            "all",
+            PkgVersion.parse(bottle_hash["formula"]["pkg_version"]),
+            Utils::Bottles::Tag.from_symbol(:all),
             bottle_hash["bottle"]["rebuild"],
           )
 

Library/Homebrew/download_strategy.rb

@@ -197,7 +197,7 @@ class VCSDownloadStrategy < AbstractDownloadStrategy
     super
     @ref_type, @ref = extract_ref(meta)
     @revision = meta[:revision]
-    @cached_location = @cache/"#{name}--#{cache_tag}"
+    @cached_location = @cache/Utils.safe_filename("#{name}--#{cache_tag}")
   end
 
   # Download and cache the repository at {#cached_location}.
@@ -296,7 +296,7 @@ class AbstractFileDownloadStrategy < AbstractDownloadStrategy
     return @symlink_location if defined?(@symlink_location)
 
     ext = Pathname(parse_basename(url)).extname
-    @symlink_location = @cache/"#{name}--#{version}#{ext}"
+    @symlink_location = @cache/Utils.safe_filename("#{name}--#{version}#{ext}")
   end
 
   # Path for storing the completed download.
@@ -312,7 +312,7 @@ class AbstractFileDownloadStrategy < AbstractDownloadStrategy
     @cached_location = if downloads.count == 1
       downloads.first
     else
-      HOMEBREW_CACHE/"downloads/#{url_sha256}--#{resolved_basename}"
+      HOMEBREW_CACHE/"downloads/#{url_sha256}--#{Utils.safe_filename(resolved_basename)}"
     end
   end
 

Library/Homebrew/formula.rb

@@ -308,13 +308,15 @@ class Formula
   end
 
   def validate_attributes!
-    raise FormulaValidationError.new(full_name, :name, name) if name.blank? || name.match?(/\s/)
+    if name.blank? || name.match?(/\s/) || !Utils.safe_filename?(name)
+      raise FormulaValidationError.new(full_name, :name, name)
+    end
 
     url = active_spec.url
     raise FormulaValidationError.new(full_name, :url, url) if url.blank? || url.match?(/\s/)
 
     val = version.respond_to?(:to_str) ? version.to_str : version
-    return if val.present? && !val.match?(/\s/)
+    return if val.present? && !val.match?(/\s/) && Utils.safe_filename?(val)
 
     raise FormulaValidationError.new(full_name, :version, val)
   end

Library/Homebrew/software_spec.rb

@@ -295,12 +295,18 @@ class Bottle
   class Filename
     attr_reader :name, :version, :tag, :rebuild
 
+    sig { params(formula: Formula, tag: Utils::Bottles::Tag, rebuild: Integer).returns(T.attached_class) }
     def self.create(formula, tag, rebuild)
       new(formula.name, formula.pkg_version, tag, rebuild)
     end
 
+    sig { params(name: String, version: PkgVersion, tag: Utils::Bottles::Tag, rebuild: Integer).void }
     def initialize(name, version, tag, rebuild)
       @name = File.basename name
+
+      raise ArgumentError, "Invalid bottle name" unless Utils.safe_filename?(@name)
+      raise ArgumentError, "Invalid bottle version" unless Utils.safe_filename?(version.to_s)
+
       @version = version
       @tag = tag.to_s
       @rebuild = rebuild

Library/Homebrew/test/bottle_filename_spec.rb

@@ -7,47 +7,47 @@ describe Bottle::Filename do
   subject { described_class.new(name, version, tag, rebuild) }
 
   let(:name) { "user/repo/foo" }
-  let(:version) { "1.0" }
-  let(:tag) { :tag }
+  let(:version) { PkgVersion.new(Version.new("1.0"), 0) }
+  let(:tag) { Utils::Bottles::Tag.from_symbol(:x86_64_linux) }
   let(:rebuild) { 0 }
 
   describe "#extname" do
-    its(:extname) { is_expected.to eq ".tag.bottle.tar.gz" }
+    its(:extname) { is_expected.to eq ".x86_64_linux.bottle.tar.gz" }
 
     context "when rebuild is 0" do
-      its(:extname) { is_expected.to eq ".tag.bottle.tar.gz" }
+      its(:extname) { is_expected.to eq ".x86_64_linux.bottle.tar.gz" }
     end
 
     context "when rebuild is 1" do
       let(:rebuild) { 1 }
 
-      its(:extname) { is_expected.to eq ".tag.bottle.1.tar.gz" }
+      its(:extname) { is_expected.to eq ".x86_64_linux.bottle.1.tar.gz" }
     end
   end
 
   describe "#to_s and #to_str" do
-    its(:to_s) { is_expected.to eq "foo--1.0.tag.bottle.tar.gz" }
-    its(:to_str) { is_expected.to eq "foo--1.0.tag.bottle.tar.gz" }
+    its(:to_s) { is_expected.to eq "foo--1.0.x86_64_linux.bottle.tar.gz" }
+    its(:to_str) { is_expected.to eq "foo--1.0.x86_64_linux.bottle.tar.gz" }
   end
 
   describe "#url_encode" do
-    its(:url_encode) { is_expected.to eq "foo-1.0.tag.bottle.tar.gz" }
+    its(:url_encode) { is_expected.to eq "foo-1.0.x86_64_linux.bottle.tar.gz" }
   end
 
   describe "#github_packages" do
-    its(:github_packages) { is_expected.to eq "foo--1.0.tag.bottle.tar.gz" }
+    its(:github_packages) { is_expected.to eq "foo--1.0.x86_64_linux.bottle.tar.gz" }
   end
 
   describe "#json" do
-    its(:json) { is_expected.to eq "foo--1.0.tag.bottle.json" }
+    its(:json) { is_expected.to eq "foo--1.0.x86_64_linux.bottle.json" }
 
     context "when rebuild is 1" do
-      its(:json) { is_expected.to eq "foo--1.0.tag.bottle.json" }
+      its(:json) { is_expected.to eq "foo--1.0.x86_64_linux.bottle.json" }
     end
   end
 
   describe "::create" do
-    subject { described_class.create(f, :tag, 0) }
+    subject { described_class.create(f, tag, rebuild) }
 
     let(:f) do
       formula do
@@ -56,6 +56,6 @@ describe Bottle::Filename do
       end
     end
 
-    its(:to_s) { is_expected.to eq "formula_name--1.0.tag.bottle.tar.gz" }
+    its(:to_s) { is_expected.to eq "formula_name--1.0.x86_64_linux.bottle.tar.gz" }
   end
 end

Library/Homebrew/utils.rb

@@ -168,4 +168,17 @@ module Utils
     word.downcase!
     word
   end
+
+  SAFE_FILENAME_REGEX = /[[:cntrl:]#{Regexp.escape("#{File::SEPARATOR}#{File::ALT_SEPARATOR}")}]/o
+  private_constant :SAFE_FILENAME_REGEX
+
+  sig { params(basename: String).returns(T::Boolean) }
+  def self.safe_filename?(basename)
+    !SAFE_FILENAME_REGEX.match?(basename)
+  end
+
+  sig { params(basename: String).returns(String) }
+  def self.safe_filename(basename)
+    basename.gsub(SAFE_FILENAME_REGEX, "")
+  end
 end